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))
+        {
+            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;
-- 
2.25.4

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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