6 ms·

>The xfce port has already been modified to accommodate this. Where are the diffs?

by veave 3y ago

SuperSandro2000 3y ago
The links also don't contain it. Mailing list based workflows....
5e92cb50239222b 3y ago
Oh, the irony of linking to GitHub. https://github.com/openbsd/ports/commit/bf33ea5f3ff390d8cde3887a48948e8acef43335 https://github.com/openbsd/ports/commit/bf33ea5f3ff390d8cde3... https://github.com/openbsd/ports/commit/469b0a4b0a1152576d93f5797a3208c1efdc80cc https://github.com/openbsd/ports/commit/469b0a4b0a1152576d93...
- kouteiheika 3y ago
  > https://github.com/openbsd/ports/commit/bf33ea5f3ff390d8cde3887a48948e8acef43335 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.
  - deleted 3y ago
    [deleted]
  - smcl 3y ago
    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".
    - jraph 3y ago
      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…
      - smcl 3y ago
        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.
        jraph 3y ago
        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.
        smcl 3y ago
        Oh yeah, don't worry we have all been there :)
    - xxpor 3y ago
      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.
  - its-summertime 3y ago
    Wouldn't that mean that `operator` would match `operator1234` since it doesn't compare the NUL?
  - daneel_w 3y ago
    > "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>".
  - thesuitonym 3y ago
    >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.
    - Karellen 3y ago
      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 https://www.openbsd.org/faq/ports/guide.html#PortsChecklist
  - Freaky 3y ago
    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.
  - tedunangst 3y ago
    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.
    - sltkr 3y ago
      How is that obvious? Isn't it more likely that the author forgot to account for the terminating zero?
      - tedunangst 3y ago
        Yeah, strncmp is the wrong function, but it was already wrong before the recent change.

>The xfce port has already been modified to accommodate this. Where are the diffs?

by veave 3y ago

>The xfce port has already been modified to accommodate this. Where are the diffs?

SuperSandro2000 3y ago

The links also don't contain it. Mailing list based workflows....

5e92cb50239222b 3y ago

Oh, the irony of linking to GitHub. https://github.com/openbsd/ports/commit/bf33ea5f3ff390d8cde3887a48948e8acef43335 https://github.com/openbsd/ports/commit/bf33ea5f3ff390d8cde3... https://github.com/openbsd/ports/commit/469b0a4b0a1152576d93f5797a3208c1efdc80cc https://github.com/openbsd/ports/commit/469b0a4b0a1152576d93...

kouteiheika 3y ago
> https://github.com/openbsd/ports/commit/bf33ea5f3ff390d8cde3887a48948e8acef43335 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.
- deleted 3y ago
  [deleted]
- smcl 3y ago
  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".
  - jraph 3y ago
    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…
    - smcl 3y ago
      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.
      - jraph 3y ago
        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.
        smcl 3y ago
        Oh yeah, don't worry we have all been there :)
  - xxpor 3y ago
    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.
- its-summertime 3y ago
  Wouldn't that mean that `operator` would match `operator1234` since it doesn't compare the NUL?
- daneel_w 3y ago
  > "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>".
- thesuitonym 3y ago
  >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.
  - Karellen 3y ago
    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 https://www.openbsd.org/faq/ports/guide.html#PortsChecklist
- Freaky 3y ago
  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.
- tedunangst 3y ago
  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.
  - sltkr 3y ago
    How is that obvious? Isn't it more likely that the author forgot to account for the terminating zero?
    - tedunangst 3y ago
      Yeah, strncmp is the wrong function, but it was already wrong before the recent change.