[PATCH v2] Fix bug 10775 - smbd crashes when accessing garbage filenames

[David Disseldorp]

On Mon, 1 Sep 2014 18:24:01 -0700, Jeremy Allison wrote:

> OK, here is an updated patchset that fixes this special
> case inside the srvstr_push_fn() code - which I think
> is the right way to do it until we fix all the underlying
> infrastructure.
> 
> It detects the (len == 0 && dest_len != 0 && *str != '\0') case
> and returns NT_STATUS_UNSUCCESSFUL. I think that should
> catch it. I also added an explicit comment and a
> FIXME.
> 
> It's ugly, but the best I can do at the moment
> until we fix all the underlying push_XXXX functions
> to correctly return error codes, not lengths - which
> is the correct long term fix.
> 
> I know this special-casing is ugly, but it's isolated
> in one function (srvstr_push_fn) - and all this will
> get removed once the underlying fix goes in :-).
> 
> Updated patchset attached (the only change from
> the previous one is the afore-mentioned change to the:
> 
> s3: smbd: Change the function signature of srvstr_push() from returning a length to returning an NTSTATUS with a length param.

 70         } else if (len == 0 && dest_len != 0 && *src != '\0') {
 71                 /*
 72                  * Underlying code path that returned
 73                  * 0 on error, without setting errno.
 74                  * push_string_base() -> push_ascii() -> strupper_m()
 75                  * for example. Generic error return.
 76                  * FIXME - tidy this when all underlying
 77                  * push_XXX conversions return error codes.
 78                  */
 79                 status = NT_STATUS_UNSUCCESSFUL;
 80                 DEBUG(10,("character conversion failure "
 81                         "on string (%s)\n",
 82                         src));
 83         }

I weeded through the direct (and many indirect) strupper_m() callers.
None of them are doing anything funky with errno, so I think it'd be
cleaner to just have strupper_m() set errno on error and drop this
block.

One other comment regarding the illegal char filters, e.g.
smb2_find.c:

440                 status = smbd_dirptr_lanman2_entry(state,
...
462                 if (!NT_STATUS_IS_OK(status)) {
463                         if (NT_STATUS_EQUAL(status, NT_STATUS_ILLEGAL_CHARACTER)) {
464                                 /*
465                                  * Bad character conversion on name. Ignore this
466                                  * entry.
467                                  */
468                                 continue;

My preference would be to filter the illegal paths alongside the other
veto/hide logic in is_visible_file() or is_legal_name().
Aside from the benefit of having filtering done in (mostly) the one
place, it'd also ensure that illegal paths aren't added to the directory
cache.

Cheers, David