Hacker News Reader

Comment by kouteiheika | Hacker News Reader

> https://github.com/openbsd/ports/commit/bf33ea5f3ff390d8cde3...

Now, this is surprising. I randomly clicked on that link and I immediately see that the code and the patch has a bug. It only checks the first 8 characters:

    -      if (gr != NULL && strncmp(gr->gr_name, "operator", 8) == 0)
    +      if (gr != NULL && strncmp(gr->gr_name, "_shutdown", 8) == 0)

so it will also match a group named "_shutdow" or "_shutdow1234" because only the first 8 characters matter.

A trivial silly mistake like this is... not what I expected considering OpenBSD's reputation for security.

View on HN

> "A trivial silly mistake like this is... not what I expected considering OpenBSD's reputation for security."

Forgetting that "_shutdown" is 9 characters is definitely an oversight on their behalf. But you need superuser privileges to create groups, and at that point you can also just toss any user right into the regular _shutdow(n) group.

There's not really anything to exploit here, but if this type of comparison is present in other software - whether OpenBSD's system tools or not - I can definitely see how it has a general potential to cause a sysop to accidentally "cross-pollinate" his or her users, e.g. "ftpuser<some digits>".

Is this not just a cosmetic thing? Xfce wants to know if you have shutdown permissions so it can render the right buttons for what you can do, but so long as the code isn't running privileged it's not actually going to let you do anything — it'll just run /sbin/shutdown fruitlessly.

>A trivial silly mistake like this is... not what I expected considering OpenBSD's reputation for security.

It may be a comfort to know that XFCE is not part of OpenBSD, then.

But that change isn't in upstream XFCE.

It's in the OpenBSD ports CVS tree, as can be seen by the fact that it's adding the file `x11/xfce4/xfce4-session/patches/patch-xfce4-session_xfsm-shutdown-fallback_c` and `/patches/` means it's part of the OpenBSD-specific `ports` infrastructure.[0]

The change might be submitted and accepted upstream in the future, but I think it's hard to justify this particular change as not being part of OpenBSD at the moment.

[0] https://www.openbsd.org/faq/ports/guide.html#PortsChecklist

Wouldn't that mean that `operator` would match `operator1234` since it doesn't compare the NUL?

It's a long-shot but I wonder if OpenBSD has specified that groups are only unique up to 8 characters.

Like it certainly looks like a bug to my eyes too, but my default when I see something like this in OpenBSD code is "maybe I just don't get it".

If OpenBSD had specified that groups are only unique up to 8 characters, wouldn't they avoid to specify a "_shutdown" group of higher length?

Looks like an overlook to me. Double click on operator, yank, type the new group name, done. Or even a quick replace all not noticing there was an hardcoded length right after one of the occurrences. The parent might have found an actual issue.

But happy to join you in the "maybe I just don't get it" clan.

You still need to be root to create groups and assign them to users, so probably not a big one and it will probably work all the time in practice.

How funny is this, someone randomly asks for the diff on HN for no apparent reason, someone else gives a link, someone else randomly opens the link and finds a potential bug.

Eyeballs, shallow bugs…

Looking back at the change after lunch it does seem like it was a rename without realising the strings were a different length. Actually changing "operator" to "shutdown", confirming the length was the same and didn't need to change, then realising it needs to be prefixed with "_" but not updating the length param would explain this nicely.

Yep! I like this theory.

Related: how many times I introduced bugs after some quick refactor on some careful code to make a linter happy.

Yes, yes, tests, I know.

Oh yeah, don't worry we have all been there :)

If that's the case, there should be a #define for that length.

If not, they should be using strlen("_shutdown") or sizeof("_shutdown")+1

There's really no excuse for a magic number there.

you'd want strlen("_shutdown")+1, since checking without the null-terminator means that you match with strings like '_shutdown123'.

It's one of the reasons why I detest the strcmp warning, as strcmp is perfectly safe given a literal string as a comparison, the comparison will drop out as not matching whenever the first NUL happens, and a literal string acts the same as specifying 'n'. Blind usage of strncmp leads to perhaps even more bugs than blind usage of strcmp.

That would introduce a bug. See the following:

    #include <stdio.h>
    int main()
    {
        printf("%lu\n", strlen("_shutdown"));
        printf("%lu\n", sizeof("_shutdown"));
        return 0;
    }

This prints the numbers 9 and 10 (on gcc I have). So if you used `sizeof(...)+1` when calculating the length argument for strcmp you'd be comparing the 9-byte string, the null terminator and one character of random nonsense.

facepalm, should have been strlen+1, not sizeof+1

Yeah naming things and off-by-one errors - the three most common bugs in CS ;)

It also previously matched operator9000, so the intent was obviously a simple prefix check, and thus preserving that behavior is essential to avoid breaking userland.

How is that obvious? Isn't it more likely that the author forgot to account for the terminating zero?

Yeah, strncmp is the wrong function, but it was already wrong before the recent change.

Reply on news.ycombinator.com