4 ms·

Their assessment seems fair to me. There are lots of reasons that software can be bad. "Easy to misuse" is one of them. Not the same thing as a vulnerability

Their assessment seems fair to me. There are lots of reasons that software can be bad. "Easy to misuse" is one of them. Not the same thing as a vulnerability. I'm sure there are some Rust devs who would say the same thing about C. It's possible to write secure code in C, just as it's possible to write a sort with defined behavior using glibc.

sgift 3y ago
The reason I'd say assigning a CVE is the right call is that the "easy to misuse" leads to a potential security problem here because of the way glibc handles the function that you give it. Case in point: You can make the same error in Java (I think it's even included in the docs for Comparators that you shouldn't do a-b, because of integer overflow/underflow), but it cannot lead to an out-of-bounds read/write. And that's because Java (~glibc) handles the comparison function that you give it differently.
- hedora 3y ago
  It can't lead to an out of bounds write, but it could violate higher-level invariants in the Java program, and those could be exploitable. For instance, the bad comparator could be used in nested authentication checks: if check_A { drop privilege to FOO } else if check_B { drop privilege to BAR } else { /* leave root bit set, because "not (A or B)" means root */ } and it could lead to a privilege escalation in the face of malformed inputs. I've seen this sort of thing happen in the wild with Java.
skitter 3y ago
I don't think it's undefined behavior though. Looking at (the working draft of) the C ISO standard, I don't see a requirement that the comparison function must be transitive. The closes thing there is 7.22.5.2 §4: "If two elements compare as equal, their order in the resulting sorted array is unspecified" So I'd say blaming users for a bug in glibc isn't fair.
- int_19h 3y ago
  In ISO C17, 7.2.5/4 has the relevant verbiage: "When the same objects (consisting of size bytes, irrespective of their current positions in the array) are passed more than once to the comparison function, the results shall be consistent with one another. That is, for qsort they shall define a total ordering on the array, and for bsearch the same object shall always compare the same way with the key."
  - skitter 3y ago
    Thanks! So it is/was indeed UB.

sgift 3y ago

The reason I'd say assigning a CVE is the right call is that the "easy to misuse" leads to a potential security problem here because of the way glibc handles the function that you give it. Case in point: You can make the same error in Java (I think it's even included in the docs for Comparators that you shouldn't do a-b, because of integer overflow/underflow), but it cannot lead to an out-of-bounds read/write. And that's because Java (~glibc) handles the comparison function that you give it differently.

hedora 3y ago
It can't lead to an out of bounds write, but it could violate higher-level invariants in the Java program, and those could be exploitable. For instance, the bad comparator could be used in nested authentication checks: if check_A { drop privilege to FOO } else if check_B { drop privilege to BAR } else { /* leave root bit set, because "not (A or B)" means root */ } and it could lead to a privilege escalation in the face of malformed inputs. I've seen this sort of thing happen in the wild with Java.

skitter 3y ago

I don't think it's undefined behavior though. Looking at (the working draft of) the C ISO standard, I don't see a requirement that the comparison function must be transitive. The closes thing there is 7.22.5.2 §4: "If two elements compare as equal, their order in the resulting sorted array is unspecified" So I'd say blaming users for a bug in glibc isn't fair.

int_19h 3y ago
In ISO C17, 7.2.5/4 has the relevant verbiage: "When the same objects (consisting of size bytes, irrespective of their current positions in the array) are passed more than once to the comparison function, the results shall be consistent with one another. That is, for qsort they shall define a total ordering on the array, and for bsearch the same object shall always compare the same way with the key."
- skitter 3y ago
  Thanks! So it is/was indeed UB.