| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Ok, the first part with constant sized elements is done. Now, more complex ones. |
Sorry, something went wrong.
|
Looks like we have PyOS_snprintf instead of just snprintf 🤔 |
Sorry, something went wrong.
|
Almost done! There are a couple left, but I don't quite understand the code and I will highly appreciate help / advice on these cases:
|
Sorry, something went wrong.
|
@Yhg1s @gvanrossum it is ready to be reviewed :) |
Sorry, something went wrong.
|
I've made the requested changes, thanks for the suggestions 👍 |
Sorry, something went wrong.
|
@Yhg1s can you please review again? :) |
Sorry, something went wrong.
|
I've merged it with the latest main, new round of buildbot tests are required to make sure that all still works correctly. Does anyone want to take a look once again, please? 😉 |
Sorry, something went wrong.
There was a problem hiding this comment.
This now has merge conflicts
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
sprintf is indeed insecure generally: https://rules.sonarsource.com/c/RSPEC-6069 But, for our cases - I was not able to find a single insecure place, so I don't mark this PR as :security:
In the past, I saw people introducing bugs and security vulnerabilities by attempting to make linters happy about getting rid of "dangerous functions".
Python provides advances functions to format strings in a safe way: PyBytes_FromFormat() and PyUnicode_FromFormat(). Moreover, there is also _PyBytesWriter and _PyUnicodeWriter internal API which can be used to "create" a new string in a safe way (without having to handle memory directly).
If a string is longer than expected, PyOS_snprintf() truncates the string, but your change ignores PyOS_snprintf() result and so the string is ignored silently. I am not convinced that these changes are worth it:
For example, the PR changes FindAddress() to use PyOS_snprintf(), but it keeps alloca() which is IMO way more problematic. Using PyBytes_FromFormat() would be safer here: the function takes care of memory management and allocates memory on the heap, there is no risk of stack overflow.
If you want me to review this PR, I would prefer smaller PRs with a more precise scope and goal.
Sorry, something went wrong.
The worst example that I recall:
Commit message of the fix: commit 4f980905a0bff94807ea07cb897c0e4cd4e6b83f
Author: Stanislav Malyshev <stas@php.net>
Date: Fri Aug 19 22:49:18 2011 +0000
Unbreak crypt() (fix bug #55439)
# If you want to remove static analyser messages, be my guest,
# but please run unit tests after
Well, at that time, PHP had no automated test suite, and running tests showed the regression. But well, it's surprising how using "safer" function can introduce a major security vulnerability. The bug allowed to bypass any kind of login page (implemented with crypt()) with any password... |
Sorry, something went wrong.
|
I wanted to close this PR anyway, I think that we should do it on a per-usage approach. Easier to review, easier to merge. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Several points of interest: