* [PATCH] ksmbd: prevent out of share access
@ 2021-09-17 10:49 Hyunchul Lee
2021-09-17 13:18 ` Namjae Jeon
0 siblings, 1 reply; 3+ messages in thread
From: Hyunchul Lee @ 2021-09-17 10:49 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, lsahlber, kernel-team, Namjae Jeon
Because of "..", files outside the share directory
could be accessed. To prevent this, normalize
the given path and remove all "." and ".."
components.
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/misc.c | 79 ++++++++++++++++++++++++++++++++++++++++------
fs/ksmbd/misc.h | 3 +-
fs/ksmbd/smb2pdu.c | 14 +++++---
3 files changed, 80 insertions(+), 16 deletions(-)
diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
index 0b307ca28a19..d599cb686415 100644
--- a/fs/ksmbd/misc.c
+++ b/fs/ksmbd/misc.c
@@ -191,19 +191,80 @@ int get_nlink(struct kstat *st)
return nlink;
}
-void ksmbd_conv_path_to_unix(char *path)
+char *ksmbd_conv_path_to_unix(char *path)
{
+ size_t path_len, remain_path_len, out_path_len;
+ char *out_path, *out_next;
+ int i, pre_dotdot_cnt = 0, slash_cnt = 0;
+ bool is_last;
+
strreplace(path, '\\', '/');
-}
+ path_len = strlen(path);
+ remain_path_len = path_len;
+ if (path_len == 0)
+ return ERR_PTR(-EINVAL);
-void ksmbd_strip_last_slash(char *path)
-{
- int len = strlen(path);
+ out_path = kzalloc(path_len + 2, GFP_KERNEL);
+ if (!out_path)
+ return ERR_PTR(-ENOMEM);
+ out_path_len = 0;
+ out_next = out_path;
+
+ do {
+ char *name = path + path_len - remain_path_len;
+ char *next = strchrnul(name, '/');
+ size_t name_len = next - name;
+
+ is_last = !next[0];
+ if (name_len == 2 && name[0] == '.' && name[1] == '.') {
+ pre_dotdot_cnt++;
+ /* handle the case that path ends with "/.." */
+ if (is_last)
+ goto follow_dotdot;
+ } else {
+ if (pre_dotdot_cnt) {
+follow_dotdot:
+ slash_cnt = 0;
+ for (i = out_path_len - 1; i >= 0; i--) {
+ if (out_path[i] == '/') {
+ slash_cnt++;
+ if (slash_cnt ==
+ pre_dotdot_cnt + 1)
+ break;
+ }
+ }
+
+ if (i < 0 &&
+ slash_cnt != pre_dotdot_cnt) {
+ kfree(out_path);
+ return ERR_PTR(-EINVAL);
+ }
+
+ out_next = &out_path[i+1];
+ *out_next = '\0';
+ out_path_len = i + 1;
- while (len && path[len - 1] == '/') {
- path[len - 1] = '\0';
- len--;
- }
+ }
+
+ if (name_len != 0 &&
+ !(name_len == 1 && name[0] == '.') &&
+ !(name_len == 2 && name[0] == '.' && name[1] == '.')) {
+ next[0] = '\0';
+ sprintf(out_next, "%s/", name);
+ out_next += name_len + 1;
+ out_path_len += name_len + 1;
+ next[0] = '/';
+ }
+ pre_dotdot_cnt = 0;
+ }
+
+ remain_path_len -= name_len + 1;
+ } while (!is_last);
+
+ if (out_path_len > 0)
+ out_path[out_path_len-1] = '\0';
+ path[path_len] = '\0';
+ return out_path;
}
void ksmbd_conv_path_to_windows(char *path)
diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
index af8717d4d85b..b7b10139ada2 100644
--- a/fs/ksmbd/misc.h
+++ b/fs/ksmbd/misc.h
@@ -16,8 +16,7 @@ int ksmbd_validate_filename(char *filename);
int parse_stream_name(char *filename, char **stream_name, int *s_type);
char *convert_to_nt_pathname(char *filename, char *sharepath);
int get_nlink(struct kstat *st);
-void ksmbd_conv_path_to_unix(char *path);
-void ksmbd_strip_last_slash(char *path);
+char *ksmbd_conv_path_to_unix(char *path);
void ksmbd_conv_path_to_windows(char *path);
char *ksmbd_extract_sharename(char *treename);
char *convert_to_unix_name(struct ksmbd_share_config *share, char *name);
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c86164dc70bb..46e0275a77a8 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -634,7 +634,7 @@ static char *
smb2_get_name(struct ksmbd_share_config *share, const char *src,
const int maxlen, struct nls_table *local_nls)
{
- char *name, *unixname;
+ char *name, *norm_name, *unixname;
name = smb_strndup_from_utf16(src, maxlen, 1, local_nls);
if (IS_ERR(name)) {
@@ -643,11 +643,15 @@ smb2_get_name(struct ksmbd_share_config *share, const char *src,
}
/* change it to absolute unix name */
- ksmbd_conv_path_to_unix(name);
- ksmbd_strip_last_slash(name);
-
- unixname = convert_to_unix_name(share, name);
+ norm_name = ksmbd_conv_path_to_unix(name);
+ if (IS_ERR(norm_name)) {
+ kfree(name);
+ return norm_name;
+ }
kfree(name);
+
+ unixname = convert_to_unix_name(share, norm_name);
+ kfree(norm_name);
if (!unixname) {
pr_err("can not convert absolute name\n");
return ERR_PTR(-ENOMEM);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] ksmbd: prevent out of share access
2021-09-17 10:49 [PATCH] ksmbd: prevent out of share access Hyunchul Lee
@ 2021-09-17 13:18 ` Namjae Jeon
2021-09-17 14:11 ` Steve French
0 siblings, 1 reply; 3+ messages in thread
From: Namjae Jeon @ 2021-09-17 13:18 UTC (permalink / raw)
To: Hyunchul Lee; +Cc: linux-cifs, Steve French, lsahlber, kernel-team
2021-09-17 19:49 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> Because of "..", files outside the share directory
> could be accessed. To prevent this, normalize
> the given path and remove all "." and ".."
> components.
>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/misc.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> fs/ksmbd/misc.h | 3 +-
> fs/ksmbd/smb2pdu.c | 14 +++++---
> 3 files changed, 80 insertions(+), 16 deletions(-)
>
> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
> index 0b307ca28a19..d599cb686415 100644
> --- a/fs/ksmbd/misc.c
> +++ b/fs/ksmbd/misc.c
> @@ -191,19 +191,80 @@ int get_nlink(struct kstat *st)
> return nlink;
> }
>
> -void ksmbd_conv_path_to_unix(char *path)
> +char *ksmbd_conv_path_to_unix(char *path)
> {
> + size_t path_len, remain_path_len, out_path_len;
> + char *out_path, *out_next;
> + int i, pre_dotdot_cnt = 0, slash_cnt = 0;
> + bool is_last;
> +
> strreplace(path, '\\', '/');
> -}
> + path_len = strlen(path);
> + remain_path_len = path_len;
> + if (path_len == 0)
> + return ERR_PTR(-EINVAL);
>
> -void ksmbd_strip_last_slash(char *path)
> -{
> - int len = strlen(path);
> + out_path = kzalloc(path_len + 2, GFP_KERNEL);
> + if (!out_path)
> + return ERR_PTR(-ENOMEM);
> + out_path_len = 0;
> + out_next = out_path;
> +
> + do {
> + char *name = path + path_len - remain_path_len;
> + char *next = strchrnul(name, '/');
> + size_t name_len = next - name;
> +
> + is_last = !next[0];
> + if (name_len == 2 && name[0] == '.' && name[1] == '.') {
> + pre_dotdot_cnt++;
> + /* handle the case that path ends with "/.." */
> + if (is_last)
> + goto follow_dotdot;
> + } else {
> + if (pre_dotdot_cnt) {
> +follow_dotdot:
> + slash_cnt = 0;
> + for (i = out_path_len - 1; i >= 0; i--) {
> + if (out_path[i] == '/') {
> + slash_cnt++;
> + if (slash_cnt ==
> + pre_dotdot_cnt + 1)
> + break;
> + }
checkpatch.pl warn:
WARNING: Too many leading tabs - consider code refactoring
#70: FILE: fs/ksmbd/misc.c:231:
+ if (slash_cnt ==
total: 0 errors, 1 warnings, 125 lines checked
updated like this.
if (out_path[i] == '/' &&
++slash_cnt == pre_dotdot_cnt + 1)
break;
Thanks.
> + }
> +
> + if (i < 0 &&
> + slash_cnt != pre_dotdot_cnt) {
> + kfree(out_path);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + out_next = &out_path[i+1];
> + *out_next = '\0';
> + out_path_len = i + 1;
>
> - while (len && path[len - 1] == '/') {
> - path[len - 1] = '\0';
> - len--;
> - }
> + }
> +
> + if (name_len != 0 &&
> + !(name_len == 1 && name[0] == '.') &&
> + !(name_len == 2 && name[0] == '.' && name[1] == '.')) {
> + next[0] = '\0';
> + sprintf(out_next, "%s/", name);
> + out_next += name_len + 1;
> + out_path_len += name_len + 1;
> + next[0] = '/';
> + }
> + pre_dotdot_cnt = 0;
> + }
> +
> + remain_path_len -= name_len + 1;
> + } while (!is_last);
> +
> + if (out_path_len > 0)
> + out_path[out_path_len-1] = '\0';
> + path[path_len] = '\0';
> + return out_path;
> }
>
> void ksmbd_conv_path_to_windows(char *path)
> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
> index af8717d4d85b..b7b10139ada2 100644
> --- a/fs/ksmbd/misc.h
> +++ b/fs/ksmbd/misc.h
> @@ -16,8 +16,7 @@ int ksmbd_validate_filename(char *filename);
> int parse_stream_name(char *filename, char **stream_name, int *s_type);
> char *convert_to_nt_pathname(char *filename, char *sharepath);
> int get_nlink(struct kstat *st);
> -void ksmbd_conv_path_to_unix(char *path);
> -void ksmbd_strip_last_slash(char *path);
> +char *ksmbd_conv_path_to_unix(char *path);
> void ksmbd_conv_path_to_windows(char *path);
> char *ksmbd_extract_sharename(char *treename);
> char *convert_to_unix_name(struct ksmbd_share_config *share, char *name);
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index c86164dc70bb..46e0275a77a8 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -634,7 +634,7 @@ static char *
> smb2_get_name(struct ksmbd_share_config *share, const char *src,
> const int maxlen, struct nls_table *local_nls)
> {
> - char *name, *unixname;
> + char *name, *norm_name, *unixname;
>
> name = smb_strndup_from_utf16(src, maxlen, 1, local_nls);
> if (IS_ERR(name)) {
> @@ -643,11 +643,15 @@ smb2_get_name(struct ksmbd_share_config *share, const
> char *src,
> }
>
> /* change it to absolute unix name */
> - ksmbd_conv_path_to_unix(name);
> - ksmbd_strip_last_slash(name);
> -
> - unixname = convert_to_unix_name(share, name);
> + norm_name = ksmbd_conv_path_to_unix(name);
> + if (IS_ERR(norm_name)) {
> + kfree(name);
> + return norm_name;
> + }
> kfree(name);
> +
> + unixname = convert_to_unix_name(share, norm_name);
> + kfree(norm_name);
> if (!unixname) {
> pr_err("can not convert absolute name\n");
> return ERR_PTR(-ENOMEM);
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] ksmbd: prevent out of share access
2021-09-17 13:18 ` Namjae Jeon
@ 2021-09-17 14:11 ` Steve French
0 siblings, 0 replies; 3+ messages in thread
From: Steve French @ 2021-09-17 14:11 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Hyunchul Lee, linux-cifs, Ronnie Sahlberg, kernel-team
Namjae's updated version merged into smb3-kernel cifsd-for-next branch.
Also starting additional tests on it
On Fri, Sep 17, 2021 at 8:18 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 2021-09-17 19:49 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > Because of "..", files outside the share directory
> > could be accessed. To prevent this, normalize
> > the given path and remove all "." and ".."
> > components.
> >
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> > ---
> > fs/ksmbd/misc.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> > fs/ksmbd/misc.h | 3 +-
> > fs/ksmbd/smb2pdu.c | 14 +++++---
> > 3 files changed, 80 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
> > index 0b307ca28a19..d599cb686415 100644
> > --- a/fs/ksmbd/misc.c
> > +++ b/fs/ksmbd/misc.c
> > @@ -191,19 +191,80 @@ int get_nlink(struct kstat *st)
> > return nlink;
> > }
> >
> > -void ksmbd_conv_path_to_unix(char *path)
> > +char *ksmbd_conv_path_to_unix(char *path)
> > {
> > + size_t path_len, remain_path_len, out_path_len;
> > + char *out_path, *out_next;
> > + int i, pre_dotdot_cnt = 0, slash_cnt = 0;
> > + bool is_last;
> > +
> > strreplace(path, '\\', '/');
> > -}
> > + path_len = strlen(path);
> > + remain_path_len = path_len;
> > + if (path_len == 0)
> > + return ERR_PTR(-EINVAL);
> >
> > -void ksmbd_strip_last_slash(char *path)
> > -{
> > - int len = strlen(path);
> > + out_path = kzalloc(path_len + 2, GFP_KERNEL);
> > + if (!out_path)
> > + return ERR_PTR(-ENOMEM);
> > + out_path_len = 0;
> > + out_next = out_path;
> > +
> > + do {
> > + char *name = path + path_len - remain_path_len;
> > + char *next = strchrnul(name, '/');
> > + size_t name_len = next - name;
> > +
> > + is_last = !next[0];
> > + if (name_len == 2 && name[0] == '.' && name[1] == '.') {
> > + pre_dotdot_cnt++;
> > + /* handle the case that path ends with "/.." */
> > + if (is_last)
> > + goto follow_dotdot;
> > + } else {
> > + if (pre_dotdot_cnt) {
> > +follow_dotdot:
> > + slash_cnt = 0;
> > + for (i = out_path_len - 1; i >= 0; i--) {
> > + if (out_path[i] == '/') {
> > + slash_cnt++;
> > + if (slash_cnt ==
> > + pre_dotdot_cnt + 1)
> > + break;
> > + }
> checkpatch.pl warn:
>
> WARNING: Too many leading tabs - consider code refactoring
> #70: FILE: fs/ksmbd/misc.c:231:
> + if (slash_cnt ==
>
> total: 0 errors, 1 warnings, 125 lines checked
>
> updated like this.
>
> if (out_path[i] == '/' &&
> ++slash_cnt == pre_dotdot_cnt + 1)
> break;
> Thanks.
>
> > + }
> > +
> > + if (i < 0 &&
> > + slash_cnt != pre_dotdot_cnt) {
> > + kfree(out_path);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + out_next = &out_path[i+1];
> > + *out_next = '\0';
> > + out_path_len = i + 1;
> >
> > - while (len && path[len - 1] == '/') {
> > - path[len - 1] = '\0';
> > - len--;
> > - }
> > + }
> > +
> > + if (name_len != 0 &&
> > + !(name_len == 1 && name[0] == '.') &&
> > + !(name_len == 2 && name[0] == '.' && name[1] == '.')) {
> > + next[0] = '\0';
> > + sprintf(out_next, "%s/", name);
> > + out_next += name_len + 1;
> > + out_path_len += name_len + 1;
> > + next[0] = '/';
> > + }
> > + pre_dotdot_cnt = 0;
> > + }
> > +
> > + remain_path_len -= name_len + 1;
> > + } while (!is_last);
> > +
> > + if (out_path_len > 0)
> > + out_path[out_path_len-1] = '\0';
> > + path[path_len] = '\0';
> > + return out_path;
> > }
> >
> > void ksmbd_conv_path_to_windows(char *path)
> > diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
> > index af8717d4d85b..b7b10139ada2 100644
> > --- a/fs/ksmbd/misc.h
> > +++ b/fs/ksmbd/misc.h
> > @@ -16,8 +16,7 @@ int ksmbd_validate_filename(char *filename);
> > int parse_stream_name(char *filename, char **stream_name, int *s_type);
> > char *convert_to_nt_pathname(char *filename, char *sharepath);
> > int get_nlink(struct kstat *st);
> > -void ksmbd_conv_path_to_unix(char *path);
> > -void ksmbd_strip_last_slash(char *path);
> > +char *ksmbd_conv_path_to_unix(char *path);
> > void ksmbd_conv_path_to_windows(char *path);
> > char *ksmbd_extract_sharename(char *treename);
> > char *convert_to_unix_name(struct ksmbd_share_config *share, char *name);
> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> > index c86164dc70bb..46e0275a77a8 100644
> > --- a/fs/ksmbd/smb2pdu.c
> > +++ b/fs/ksmbd/smb2pdu.c
> > @@ -634,7 +634,7 @@ static char *
> > smb2_get_name(struct ksmbd_share_config *share, const char *src,
> > const int maxlen, struct nls_table *local_nls)
> > {
> > - char *name, *unixname;
> > + char *name, *norm_name, *unixname;
> >
> > name = smb_strndup_from_utf16(src, maxlen, 1, local_nls);
> > if (IS_ERR(name)) {
> > @@ -643,11 +643,15 @@ smb2_get_name(struct ksmbd_share_config *share, const
> > char *src,
> > }
> >
> > /* change it to absolute unix name */
> > - ksmbd_conv_path_to_unix(name);
> > - ksmbd_strip_last_slash(name);
> > -
> > - unixname = convert_to_unix_name(share, name);
> > + norm_name = ksmbd_conv_path_to_unix(name);
> > + if (IS_ERR(norm_name)) {
> > + kfree(name);
> > + return norm_name;
> > + }
> > kfree(name);
> > +
> > + unixname = convert_to_unix_name(share, norm_name);
> > + kfree(norm_name);
> > if (!unixname) {
> > pr_err("can not convert absolute name\n");
> > return ERR_PTR(-ENOMEM);
> > --
> > 2.17.1
> >
> >
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-17 14:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 10:49 [PATCH] ksmbd: prevent out of share access Hyunchul Lee
2021-09-17 13:18 ` Namjae Jeon
2021-09-17 14:11 ` Steve French
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.