Use after free in ntfs_uppercase_mbs() of libntfs-3g

[jeffbencteux]

I believe there is a use after free in ntfs_uppercase_mbs() when following a specific code path:

ntfs-3g/libntfs-3g/unistr.c

Line 1193 in 1565b01

*t = 0;

It can be triggered if the call to utf8_to_unicode() in the function fails with n = -1:

                        n = utf8_to_unicode(&wc, s);

https://github.com/tuxera/ntfs-3g/blob/1565b01e215c74e5c5f83f3ecde1ed682637dc5a/libntfs-3g/unistr.c#LL1166C1-L1166C32

Execution then do not get into the branch checking for n value being positive, breaks out of the loop (for the same reason) and then the following branch is entered:

                if (n < 0) {
                        free(upp);
                        upp = (char*)NULL;
                        errno = EILSEQ;
                }

Next, an access to t is made:

                *t = 0;

Because t = upp and upp is being freed, access *t = 0; is using memory that has been freed.

I suggest removing line 1193 completely as a fix as I cannot find a legit use of it in the code.

[unsound]

Seems like you're right, but wouldn't removing *t = 0; mean the string isn't guaranteed to be NULL-terminated?
In that case I think a better solution is to enclose it in an else block:

-                *t = 0;
+                else {
+                        *t = 0;
+                }

[jeffbencteux]

Oh yes, forgot that. Your version fix it much better.

[unsound]

Closing this as the discussed change is in HEAD now: 75dcdc2

[Pastea]

@unsound I think that could be worth if this bug gets assigned a CVE so the community is aware of it, what do you think about?

[unsound]

@Pastea If there's a way to exploit this as a security issue then that would make sense, but all this use-after-free does is write a 0 byte to an area that was just freed a few instructions earlier and likely hasn't been reallocated during the course of calling 'free' (depends on the implementation details of 'free' in the libc of course, but it sounds unlikely).
I'm willing to be proven wrong, but I don't see an exploitable security issue.

[Pastea]

Yeah, the exploitation time window is very narrow due to the fact that the pointer is used only in some instruction below so forcing an allocation in that space in the meantime is quite challenging.