[phoe-krk]

A fun one. Buffer overflows tend to usually get associated with providing too much data; here's a nice case in which an overflow is triggered by providing too little. Seems like the buffer for storing the password was changed to be dynamically allocated, but only in some parts of the code; other parts still treated it as something that is at least nine bytes long (including the null terminator).

In practice, this means that if your password is only one char, then the actual buffer is two bytes long, and the seventh byte past the buffer is then zeroed/set to the null terminator. I wonder if and how this is exploitable.

[userbinator]

Seems like the buffer for storing the password was changed to be dynamically allocated

When I see things like this, the first question I have is why? A password isn't going to be long enough to require dynamic allocation, so just use a fixed-size buffer. 255 is already generous and a good round number. The best solutions are often also the simplest.

[AlexanderDhoore]

This is why I love working in embedded. Our clients would never complain about these kinds of reasonable restrictions. There are tons of limits like this in our machines, which nobody notices but make my life much easier.

I guess things are different in the Linux/PC software world.

[tgv]

It also might explain the S in IoT.

[ilyt]

I'd say not anymore, average microcontroller used in IoT got fat enough, nowadays even smaller chips come with AES acceleration too.

I'm frankly surprised some proper standard didn't pop up already, I guess closing the users in your own ecosystem as fast as possible is priority in the industry.

Good 80-90% of devices could be just "an MQTT connection + a bit of code to pair it up initially and feed server data" + a bunch of templates for how typical services should present themselves (so a light will just work with any compatible "control center"). Then just sell user subscription to your cloudy cloud IFTTT clone or a separate box to put in house that does the same job.

[raldi]

> When I see things like this, the first question I have is why?

Perhaps because they were still fighting the last war, where they were hurt by a greater-than-255-character buffer overflow vulnerability.

[insanitybit]

One reason I'd like to write passwords directly to a heap allocated buffer would be so that I can limit where that password exists in memory, ensure it's zero'd appropriately, prevent it being paged to disk, etc.

[imron]

> When I see things like this, the first question I have is why?

My default assumption is three letter agencies surreptitiously adding back doors.

[bell-cot]

With how many careless programmers there are, writing massive quantities of security-critical code...why would they bother? (Outside of a maybe a few narrow, high-value contexts.) That's like surreptitiously working to make sure that there are plenty of cat pictures on the web, and that water flows downhill.

[trashtester]

As a default assumption, this may be a bit conspiratorial. Inserting something like this into a git repo is relatively easy to track, and a given "contributor" could not do many such things without being caught. Not saying we should ignore the possibility, though...

But there are plenty of ways for such agencies to gain similar access, including any kind of closed code in BIOSes, drivers, firmware etc. Or by taking control of select infra, and injecting MITM features there (that would remain stealthy, and only activate for very select targets.)

[Izkata]

> Not saying we should ignore the possibility, though...

Two suspected and one confirmed attempt in linux from a post 6 years ago: https://www.reddit.com/r/linux/comments/54in5s/the_nsa_has_t...

Found this while looking for a more recent one I vaguely remember involving a bad implementation of I think /dev/random

[generalizations]

This should be a reasonable position to take. There's means and motivation present. The threat model should be taken seriously.

[marcosdumay]

Because then you'll have another vulnerability created by too long passwords.

[GTP]

Not necessarily, as you can put a limit on how many characters you read from the shell. But you're right that you can screw up in both cases, I would just say that if you have a fixed length for the buffer it is a bit easier to handle it correctly.

[nicoburns]

Using a language that stores the length as part of the type is the real fix here.

[kccqzy]

So are you proposing to use dependently typed languages then? Typical languages like C++ don't make it very convenient.

    template<size_t N>
    void do_something(char (&s)[N]) { ... }

Now when you have a string of unknown length, how do you reify it with that template? In practice languages then have to keep around an unknown type argument at runtime. This is incompatible with most languages where types aren't known at runtime.

[AshamedCaptain]

Why overcomplicate things so much? He probably just meant to use std::string. Programs like this definitely don't need to care about the few dozen bytes added by std::string overhead.

[GTP]

This for sure, but the codebases that we alreaduly have in C/C++ aren't going anywhere anytime soon.

[pjmlp]

Those that are proper C++ would use a string type instead of raw C strings.

And if those developers care about security, bounds checking would be enabled.

As for C there is hardly anything we can do other than keep fixing exploits until hardware memory tagging becomes a common feature across all major platforms.

[scoutt]

You still need to compare the length against a maximum, wherever it's stored.

[GTP]

No, in that case the language will do that for you.

[_notreallyme_]

This might be exploitable in some cases. There has been a "heated" discussion in 2014 about off-by-one NUL byte heap overflow that lead to this blogpost from projectzero:

https://googleprojectzero.blogspot.com/2014/08/the-poisoned-...

There have been other examples where only a 1 could be written.

[st_goliath]

> the seventh byte past the buffer is then zeroed/set to the null terminator. I wonder if and how this is exploitable.

Well, looking at the upstream code, the original value is saved away before zeroing it out and then unconditionally restored after running crypt.

If sudo is single threaded and crypt() doesn't malloc() anything, I don't think that can be exploited. Worst case would be a segfault if the password was somehow close enough to a page boundary.

[geocar]

> I don't think that can be exploited.

glibc malloc() should be aligned to 2*sizeof(size_t), so strup("")[x] on 64-bit systems (with 16-byte alignment) can never crash or overlap another object where x<16

On 32-bit systems and with other mallocs you could potentially be reaching another page (like I think you are imagining) or trashing some bookkeeping bits which might crash free() but I cannot yet see how you would induce that, nor convince myself it cannot be done without spending more time thinking about it (something I'm reluctant to do with my afternoon)

[dottedmag]

Probably only with severely niche libc. Is there any libc that allocates data with granularity smaller than 16 bytes?

[nn3]

Very doubtful there is one. Seems like a total nothing burger

[tinus_hn]

Sudo is a security boundary, it has to be rock solid and an issue that doesn’t immediately look exploitable is still a big deal. Sudo runs under the control of the attacker, it’s playing with fire!

[ilyt]

Case in point: the whole speculative execution was only suspected to maybe be exploitable decades ago, and only now we have a bunch of PoCs

[userbinator]

...which are still only useful under highly contrived conditions which require knowing so much about the target that it wouldn't be a practical concern.

[_kbh_]

> I wonder if and how this is exploitable.

Would be at worst a memory read past the inputted password, which wouldn't be super useful outside of a leak for another vulnerability but even that seems unlikely.

[TonyTrapp]

No, the code overwrote the 9th byte of the buffer to add a null terminator: https://github.com/sudo-project/sudo/commit/bd209b9f16fcd127...

So it's not just reading past the end of the buffer, but it's overwriting a single byte potentially belonging to another object. It may still cause a crash but it's relatively unlikely that it could cause something more severe.

[_kbh_]

> No, the code overwrote the 9th byte of the buffer to add a null terminator: https://github.com/sudo-project/sudo/commit/bd209b9f16fcd127...

Ah that makes a lot more sense.

> So it's not just reading past the end of the buffer, but it's overwriting a single byte potentially belonging to another object. It may still cause a crash but it's relatively unlikely that it could cause something more severe.

Yeah I agree that nothing severe should come from it. The allocations are probably larger then the size of the buffer being used so it may not even right off the end of its own allocation.

[Someone]

I don’t understand how this can be correct:

    if (pw_len == DESLEN || HAS_AGEINFO(pw_epasswd, pw_len)) {
 strlcpy(des_pass, pass, sizeof(des_pass));
 pass = des_pass;
    }

Doesn’t that truncate your password if it happens to have DESLEN characters?

[AtNightWeCode]

It is a very known exploit. Easily found with the stupid fuzz tests but even easier found with tests that tests all edge cases. You have to have a set of complete amateurs of coders to end up with this problem in production.

[AtNightWeCode]

It is funny what you get downvoted for in this garbage forum. 15 years ago we built stuff in C and used cheap commercial available tools that detects EXACTLY this issue. There is no excuse to end up with this in prod 2022. Amateurs.

[pjmlp]

15 years? Lint was created in 1979 exactly to track down common errors in C, and until clang static analysis it was largely ignored.

No wonder that these kind of tooling keeps being ignored until we get liability in software products.