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

CodeQL alerts #406

Open
esabol opened this issue Jul 21, 2024 · 5 comments
Open

CodeQL alerts #406

esabol opened this issue Jul 21, 2024 · 5 comments

Comments

@esabol
Copy link
Member

esabol commented Jul 21, 2024

PR #404 and PR #405 fix a couple of CodeQL alerts, I think. After those, only a few CodeQL alerts remain....

The five "Potential use after free" alerts (#42, #43, #44, #45, #46) for libgearman-server/queue.cc lines 172-176 all appear to be wrong. Those lines come after the server.queue.functions= new queue_st(); line and inside the if (server.queue.functions) { ... } block, so I don't see any problem here. I think these should be dismissed as erroneous alerts. Do you agree, @SpamapS ?

The Uncontrolled format string alert around line 626 of libgearman-server/gearmand.cc appears to be legitimate. The alert suggests that, if a user sets the GEARMAND_PORT environment variable to a string with percent characters in it, for example, it could result in a buffer overflow and/or crash the program. I think this could be fixed by removing any percent characters from the buffer, right? I'm not sure that would actually satisfy CodeQL though, but it would satisfy me enough that the alert could be dismissed as handled. What do you think?

@SpamapS
Copy link
Member

SpamapS commented Aug 4, 2024

Agreed, the use after free ones seem like bugs in CodeQL. Maybe we should report them.

@SpamapS
Copy link
Member

SpamapS commented Aug 4, 2024

For the other alert, I think it doesn't matter, because the environment is essentially an administrative function, not a user function. So if they want to put % chars in there and break the daemon who are we to stop them? Anyway, it can probably be refactored to make it more robust to that and remove the footgun.

@esabol
Copy link
Member Author

esabol commented Aug 4, 2024

Agreed, the use after free ones seem like bugs in CodeQL. Maybe we should report them.

Yeah, I'll do that. OK, I have dismissed them as false positives and added comments explaining why.

I think I'll come up with a PR to truncate the GEARMAN_PORT environment variable value at the first % character.

@esabol
Copy link
Member Author

esabol commented Aug 6, 2024

I think I'll come up with a PR to truncate the GEARMAN_PORT environment variable value at the first % character.

I decided to truncate it at the first non-digit character instead since that would also address issue #320. See PR #411. Please review, @SpamapS.

@esabol
Copy link
Member Author

esabol commented Sep 20, 2024

@SpamapS : Please review the latest version of PR #411. Thanks!

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

No branches or pull requests

2 participants