Safe use of sprint/printf/fprintf in MariaDB code base
Hi all, Reading about the xz-utils backdoor authors submission of converting safe_fprintf() to fprintf() in libarchive[1] presumably in order to introduce intentional vulnerability reminded me that the MariaDB code base still has a plenty unsafe sprint/printf/fprintf use that can easily be found with scanners such as Flawfinder[2] and cppcheck[3]. There are currently 6 merge requests open by two authors (CC'd) to fix some of these issues[4]. Could we please have some more attention on these by the core contributors? If core contributors are not happy with the submissions, could you perhaps write your own safe functions (there are already some in m_string.h[5]) like many other projects seem to have (also libarchive had[6]) and then ask all contributors to use them consistently? Use of specific memory safe functions could also be mandated via the coding standards[7]. [1] https://github.com/libarchive/libarchive/pull/1609 [2] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L461-L489 [3] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L522-L554 [4] https://github.com/MariaDB/server/pulls?q=is%3Apr+is%3Aopen+sprintf+ [5] https://github.com/MariaDB/server/blob/11.5/include/m_string.h [6] https://github.com/libarchive/libarchive/blob/6110e9c82d8ba830c3440f36b99048... [7] https://github.com/MariaDB/server/blob/11.5/CODING_STANDARDS.md
Hi Otto, We agree this should be taken seriously, and I've looked over the PRs again this afternoon so that we can get them moving forward again. Of the 6: * 2 are drafts. Do you want them reviewed at this stage? * 2 I have approved, one of them I think is ready for merging, the other will need a second set of eyes. * 2 need a bit more work, one of which potentially adds new security vulnerabilities, but I'm happy to discuss them further. Point 7 of your email is a good one. You are welcome to open a PR against that, as the PR would make a good discussion point to make sure it is all there. Or someone else could add it via a PR. Kind Regards Andrew On 01/04/2024 00:52, Otto Kekäläinen via developers wrote:
Hi all,
Reading about the xz-utils backdoor authors submission of converting safe_fprintf() to fprintf() in libarchive[1] presumably in order to introduce intentional vulnerability reminded me that the MariaDB code base still has a plenty unsafe sprint/printf/fprintf use that can easily be found with scanners such as Flawfinder[2] and cppcheck[3].
There are currently 6 merge requests open by two authors (CC'd) to fix some of these issues[4]. Could we please have some more attention on these by the core contributors?
If core contributors are not happy with the submissions, could you perhaps write your own safe functions (there are already some in m_string.h[5]) like many other projects seem to have (also libarchive had[6]) and then ask all contributors to use them consistently?
Use of specific memory safe functions could also be mandated via the coding standards[7].
[1] https://github.com/libarchive/libarchive/pull/1609 [2] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L461-L489 [3] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L522-L554 [4] https://github.com/MariaDB/server/pulls?q=is%3Apr+is%3Aopen+sprintf+ [5] https://github.com/MariaDB/server/blob/11.5/include/m_string.h [6] https://github.com/libarchive/libarchive/blob/6110e9c82d8ba830c3440f36b99048... [7] https://github.com/MariaDB/server/blob/11.5/CODING_STANDARDS.md _______________________________________________ developers mailing list -- developers@lists.mariadb.org To unsubscribe send an email to developers-leave@lists.mariadb.org
-- Andrew (LinuxJedi) Hutchings Chief Contributions Officer MariaDB Foundation
Hi Andrew, Trevor and others,
We agree this should be taken seriously, and I've looked over the PRs again this afternoon so that we can get them moving forward again.
Thanks! While getting these specific PRs finalized, I suspect that having written guidance or standardized function calls in place would make the need for a lot of reviews go away.. ..so this is actually quite important:
Point 7 of your email is a good one. You are welcome to open a PR against that, as the PR would make a good discussion point to make sure it is all there. Or someone else could add it via a PR.
Sure, let's draft a PR and get the discussion going. Do you Trevor want to take a stab? I will wait some time to give others an opportunity to contribute. I don't have high confidence that my suggestion is good, but I can do it if nobody else steps up. - Otto
Hi Vicentiu! Related to this thread, Andrew just approved https://github.com/MariaDB/server/pull/2516 and marked it for your review. It could be merged right away if you are happy with it and approve. On Thu, 4 Apr 2024 at 08:13, Andrew Hutchings <andrew@mariadb.org> wrote:
Hi Otto,
We agree this should be taken seriously, and I've looked over the PRs again this afternoon so that we can get them moving forward again.
Of the 6:
* 2 are drafts. Do you want them reviewed at this stage? * 2 I have approved, one of them I think is ready for merging, the other will need a second set of eyes. * 2 need a bit more work, one of which potentially adds new security vulnerabilities, but I'm happy to discuss them further.
Point 7 of your email is a good one. You are welcome to open a PR against that, as the PR would make a good discussion point to make sure it is all there. Or someone else could add it via a PR.
Kind Regards Andrew
On 01/04/2024 00:52, Otto Kekäläinen via developers wrote:
Hi all,
Reading about the xz-utils backdoor authors submission of converting safe_fprintf() to fprintf() in libarchive[1] presumably in order to introduce intentional vulnerability reminded me that the MariaDB code base still has a plenty unsafe sprint/printf/fprintf use that can easily be found with scanners such as Flawfinder[2] and cppcheck[3].
There are currently 6 merge requests open by two authors (CC'd) to fix some of these issues[4]. Could we please have some more attention on these by the core contributors?
If core contributors are not happy with the submissions, could you perhaps write your own safe functions (there are already some in m_string.h[5]) like many other projects seem to have (also libarchive had[6]) and then ask all contributors to use them consistently?
Use of specific memory safe functions could also be mandated via the coding standards[7].
[1] https://github.com/libarchive/libarchive/pull/1609 [2] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L461-L489 [3] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L522-L554 [4] https://github.com/MariaDB/server/pulls?q=is%3Apr+is%3Aopen+sprintf+ [5] https://github.com/MariaDB/server/blob/11.5/include/m_string.h [6] https://github.com/libarchive/libarchive/blob/6110e9c82d8ba830c3440f36b99048... [7] https://github.com/MariaDB/server/blob/11.5/CODING_STANDARDS.md _______________________________________________ developers mailing list -- developers@lists.mariadb.org To unsubscribe send an email to developers-leave@lists.mariadb.org
-- Andrew (LinuxJedi) Hutchings Chief Contributions Officer MariaDB Foundation
participants (2)
-
Andrew Hutchings
-
Otto Kekäläinen