| Message ID | 20200623175534.38286-2-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vvfat: Two small patches | expand |
On 6/23/20 12:55 PM, Kevin Wolf wrote: > FAT allows only a restricted set of characters in file names, and for > some of the illegal characters, it's actually important that we catch > them: If filenames can contain '/', the guest can construct filenames > containing "../" and escape from the assigned vvfat directory. The same > problem could arise if ".." was ever accepted as a literal filename. > > Fix this by adding a check that all filenames are valid in > check_directory_consistency(). > > Reported-by: Nathan Huckleberry <nhuck15@gmail.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/vvfat.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/block/vvfat.c b/block/vvfat.c > index c65a98e3ee..2fab371258 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -520,6 +520,25 @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin) > direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff); > } > > +static bool valid_filename(const unsigned char *name) > +{ > + unsigned char c; > + if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) { > + return false; > + } > + for (; (c = *name); name++) { > + if (!((c >= '0' && c <= '9') || > + (c >= 'A' && c <= 'Z') || > + (c >= 'a' && c <= 'z') || > + c > 127 || > + strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != 0)) s/0/NULL/ Hmm - would it be any more efficient to use a single comparison of strcspn() vs. strlen(), where you merely spell out the bytes that are rejected? Out of 256 byte values, NUL is implicitly rejected (since these are C strings), the 128 high-bit bytes are all valid, and you have permitted 62 alnum and 23 other characters; that leaves merely 42 byte values to explicitly list in a reject string. Of course, writing the string literal containing those 42 invalid bytes is itself a bit of an exercise in reading the ASCII table: "\x01\x02\x03\x04\x05\x06\x07" "\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" "\x10\x11\x12\x13\x14\x15\x16\x17" "\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" " \"*/:<>?\\|\x7f" > + { > + return false; > + } > + } > + return true; > +} > + > static uint8_t to_valid_short_char(gunichar c) > { > c = g_unichar_toupper(c); > @@ -2098,6 +2117,10 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i)) > } > lfn.checksum = 0x100; /* cannot use long name twice */ > > + if (!valid_filename(lfn.name)) { > + fprintf(stderr, "Invalid file name\n"); Wow, the fact that we are still using fprintf is annoying, but pre-existing. > + goto fail; > + } > if (path_len + 1 + lfn.len >= PATH_MAX) { > fprintf(stderr, "Name too long: %s/%s\n", path, lfn.name); > goto fail; > At any rate, the idea makes sense. If you don't like my strcspn() idea, then: Reviewed-by: Eric Blake <eblake@redhat.com>
Am 23.06.2020 um 20:21 hat Eric Blake geschrieben: > On 6/23/20 12:55 PM, Kevin Wolf wrote: > > FAT allows only a restricted set of characters in file names, and for > > some of the illegal characters, it's actually important that we catch > > them: If filenames can contain '/', the guest can construct filenames > > containing "../" and escape from the assigned vvfat directory. The same > > problem could arise if ".." was ever accepted as a literal filename. > > > > Fix this by adding a check that all filenames are valid in > > check_directory_consistency(). > > > > Reported-by: Nathan Huckleberry <nhuck15@gmail.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/vvfat.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/block/vvfat.c b/block/vvfat.c > > index c65a98e3ee..2fab371258 100644 > > --- a/block/vvfat.c > > +++ b/block/vvfat.c > > @@ -520,6 +520,25 @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin) > > direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff); > > } > > +static bool valid_filename(const unsigned char *name) > > +{ > > + unsigned char c; > > + if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) { > > + return false; > > + } > > + for (; (c = *name); name++) { > > + if (!((c >= '0' && c <= '9') || > > + (c >= 'A' && c <= 'Z') || > > + (c >= 'a' && c <= 'z') || > > + c > 127 || > > + strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != 0)) > > s/0/NULL/ Ok, though this line is just copied from to_valid_short_char(). Maybe I can sneak in a (strictly speaking unrelated) change to that function to keep both consistent. > Hmm - would it be any more efficient to use a single comparison of strcspn() > vs. strlen(), where you merely spell out the bytes that are rejected? Out > of 256 byte values, NUL is implicitly rejected (since these are C strings), > the 128 high-bit bytes are all valid, and you have permitted 62 alnum and 23 > other characters; that leaves merely 42 byte values to explicitly list in a > reject string. Of course, writing the string literal containing those 42 > invalid bytes is itself a bit of an exercise in reading the ASCII table: > > "\x01\x02\x03\x04\x05\x06\x07" > "\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" > "\x10\x11\x12\x13\x14\x15\x16\x17" > "\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" > " \"*/:<>?\\|\x7f" I think this would be really hard to read. The above condition is a pretty straighforward implementation of what the spec says (even the order of characters is the same). Kevin
diff --git a/block/vvfat.c b/block/vvfat.c index c65a98e3ee..2fab371258 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -520,6 +520,25 @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin) direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff); } +static bool valid_filename(const unsigned char *name) +{ + unsigned char c; + if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) { + return false; + } + for (; (c = *name); name++) { + if (!((c >= '0' && c <= '9') || + (c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z') || + c > 127 || + strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != 0)) + { + return false; + } + } + return true; +} + static uint8_t to_valid_short_char(gunichar c) { c = g_unichar_toupper(c); @@ -2098,6 +2117,10 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i)) } lfn.checksum = 0x100; /* cannot use long name twice */ + if (!valid_filename(lfn.name)) { + fprintf(stderr, "Invalid file name\n"); + goto fail; + } if (path_len + 1 + lfn.len >= PATH_MAX) { fprintf(stderr, "Name too long: %s/%s\n", path, lfn.name); goto fail;
FAT allows only a restricted set of characters in file names, and for some of the illegal characters, it's actually important that we catch them: If filenames can contain '/', the guest can construct filenames containing "../" and escape from the assigned vvfat directory. The same problem could arise if ".." was ever accepted as a literal filename. Fix this by adding a check that all filenames are valid in check_directory_consistency(). Reported-by: Nathan Huckleberry <nhuck15@gmail.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vvfat.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)