Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix undefined behavior in dsortf comparison function #500

Closed
wants to merge 1 commit into from

Conversation

visitorckw
Copy link

In the dsortf function, when both (*e1)->name[0] == '/' and (*e2)->name[0] == '/', a situation occurs where a < b but b < a, violating the transitivity requirement for comparison functions used by qsort as specified by the C standard. This leads to undefined behavior, which can cause incorrect sorting and memory corruption in glibc's qsort implementation, creating a potential security issue [1].

Fix the issue by returning 0 when both entries begin with '/', preventing the undefined behavior.

[1] https://www.qualys.com/2024/01/30/qsort.txt

In the dsortf function, when both (*e1)->name[0] == '/' and
(*e2)->name[0] == '/', a situation occurs where a < b but b < a,
violating the transitivity requirement for comparison functions used by
qsort as specified by the C standard. This leads to undefined behavior,
which can cause incorrect sorting and memory corruption in glibc's
qsort implementation, creating a potential security issue [1].

Fix the issue by returning 0 when both entries begin with '/',
preventing the undefined behavior.

[1] https://www.qualys.com/2024/01/30/qsort.txt
@notroj
Copy link
Collaborator

notroj commented Jan 8, 2025

Thanks a lot for the PR!

@notroj
Copy link
Collaborator

notroj commented Jan 8, 2025

I think this can never happen in practice for mod_autoindex because there will only ever be one entry in the list which meets that test, so it's not a practical concern but I'll merge the fix anyway for safety.

@asfgit asfgit closed this in b5d2f5e Jan 8, 2025
@notroj
Copy link
Collaborator

notroj commented Jan 10, 2025

Review from @rpluem on dev@ here - I clearly did not think about this as long as he did 😃

https://lists.apache.org/thread/135z7yn9hjk7rly4y1r1so5rqjymgfx8

@visitorckw
Copy link
Author

Thank you for letting me know about this email!

It seems I can't reply directly to this email because I don't have an @apache.org email, and the mailing list server blocks it.
So, I'll leave my message here again:

Hi Ruediger,

On Thu, Jan 09, 2025 at 08:54:13AM +0100, Ruediger Pluem wrote:

I am fine with the change and sorry for getting into a theoretical discussion here, but I think the function before the patch was
transitive (at least regarding to the '/' topic. I have not checked other cases):

Let's assume x = '/', y = '/', z = '/'

dsortf(x, y) == -1
dsortf(y, z) == -1
dsortf(x, z) == -1

Transitive

Thanks for your review!

I initially didn't think this through carefully. However, I'll include
in the explanation that violating transitivity considers the following
case:

x = '/', y = '/', z = '/'

dsortf(x, y) == -1
dsortf(y, z) == -1
dsortf(z, x) == -1

This leads to the conclusion x < y && y < z && z < x, which might also
count as a violation of transitivity since it should ensure z > x ?

Regards,
Kuan-Wei

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants