search

AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.74k stars 430 forks source link

Security - Replace 'std::rand' #773

Closed hodoulp closed 4 years ago

hodoulp commented 4 years ago

Security issue detected by the CI build: https://sonarcloud.io/project/issues?id=imageworks_OpenColorIO&resolved=false&sinceLeakPeriod=true&types=BUG

lgritz commented 4 years ago

Is this only a "security" issue because rand() isn't a strong enough to use for crypto -- which is not at all how we're using it?

doug-walker commented 4 years ago

Excellent point Larry, the Sonar reason is "The use of srand together with rand to seed the random number generator and then generate numbers usually produces low-quality randomness." So it is not that the call itself is a security issue, just that the results aren't totally random (which we don't really care about for our usage).

michdolan commented 4 years ago

This falls into a topic worth raising at the ASWF CI working group meeting (OpenEXR had the same question recently). As to whether we answer false positives with a comment in code and/or explicitly dismissing the warning in SonarCloud for the specific cases we know are not actually an issue.

hodoulp commented 4 years ago

All the tools (from gcc warnings, unit tests to SonarCloud) are here to help us finding hidden bugs. Running these tools is the mandate of the CI build system. However, the cost is to have frequent false-positive warnings because these tools are not perfect and not tailored to the project. But disabling warnings would only inhibit more and more validations over the time and finally hide problems. At Autodesk we experience that on various projects where some warnings are disabled on part of the code (i.e. muting potential problems) and/or never addressed leading to unusable compilation output (i.e. too many warnings).

Looking at this specific case, the fix is only a copy & paste of 2 lines, so a short pull request could easily change the code to keep the check and return SonarCloud to ‘green’.