Missing catch-all in enum comparison in pkixbuild.cpp
Categories
(Core :: Security: PSM, defect)
Tracking
()
People
(Reporter: tjr, Unassigned)
Details
As a result of some static analysis work, we found a situation where we compare against at least two values of an enum, but lack a catch-all case.
This happens in https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/security/nss/lib/mozpkix/lib/pkixbuild.cpp#104-110:
if (newResult == Result::ERROR_UNTRUSTED_CERT) {
newResult = Result::ERROR_UNTRUSTED_ISSUER;
} else if (newResult == Result::ERROR_EXPIRED_CERTIFICATE) {
newResult = Result::ERROR_EXPIRED_ISSUER_CERTIFICATE;
} else if (newResult == Result::ERROR_NOT_YET_VALID_CERTIFICATE) {
newResult = Result::ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE;
}
As a result of a survey sent to assignees of sec-high bugs (you might have gotten it yourself) we are exploring static analysis queries that may help prevent future bugs. The blocking bug details one of these queries. This bug is filed (manually) as a result of running that query on m-c and randomly selecting 10 hits to investigate. (Because it would bias the experiment, no subjective analysis was done on the particular code in question.)
We'd appreciate your help in triaging this report, and indicating which of the following cases apply:
- If the code is intentional - that is there should not be a catch-all at the end and fallthrough for all current enum values is safe and bug-free, we'd ask you close this as WONTFIX
- If the code is currently safe (e.g. it checks all the enum values), but could be improved to prevent future bugs (caused by an addition to the enum) with a catch-all and a DIAGNOSTIC/RELEASE assert.
- If the code is believed safe but can't be locally asserted to be so (e.g. it handles 2 of 5 enum values and the remaining three "should never occur") - this is the most interesting case to us. If the remaining cases "should never occur" then it should be safe to add an assert, and this could lead us to prevent or catch a future functional or security bug (just like Bug 1603055).
Reporter | ||
Comment 1•3 years ago
|
||
Hey Dana, do you think you could triage this so we can finalize our statistics on the experiment?
Comment 2•3 years ago
|
||
Thanks for the reminder. This code is intentional - it modifies the error code in a few specific cases, and the catchall is essentially that the error code remains unchanged, which is what we want.
Description
•