| Message ID | 20210917104913.5823-1-hyc.lee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: prevent out of share access | expand |
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 > >
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 > > > >
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);