[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] vvfat: Check that updated filenames are valid
From: |
Eric Blake |
Subject: |
Re: [PATCH 1/2] vvfat: Check that updated filenames are valid |
Date: |
Tue, 23 Jun 2020 13:21:21 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 |
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