Re: [PATCH] ksmbd: prevent out of share access

[Steve French <smfrench@gmail.com>]

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