Closed Bug 1005256 Opened 5 years ago Closed 5 years ago

Parameter validation of mozilla::pkix::der::Input::GetMark/GetSECItem is not great

Categories

(Core :: Security: PSM, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

1. There's an implicit conversion from ptrdiff_t to unsigned int which makes VS2013 complain when the code is built with -Wall -W4.

2. We don't check that the input of the mark is the same input we're calling GetSECItem on.

3. Callers are not checking the return value.

4. Return value should be der::Result, not bool.
Duplicate of this bug: 1002921
Summary: Paramter validation of mozilla::pkix::der::Input::GetMark/GetSECItem is not great → Parameter validation of mozilla::pkix::der::Input::GetMark/GetSECItem is not great
Comment on attachment 8416686 [details] [diff] [review]
improve-GetSECItem-parameter-validation.patch

Review of attachment 8416686 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixder.h
@@ +182,5 @@
>    class Mark
>    {
>    private:
>      friend class Input;
> +    explicit Mark(const Input& input, const uint8_t* mark)

No need to mark explicit on multiple-argument constructors.

@@ +203,1 @@
>      // TODO: Return false if bounds check fails

s/false/Failure

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +423,5 @@
>    ASSERT_TRUE(item.data);
>    ASSERT_EQ(0, memcmp(item.data, expectedItemData, sizeof expectedItemData));
>  }
>  
> +// Cannot run this test on debug builds because of the PR_NOT_REACHED

I don't understand the PR_NOT_REACHED comment, is that triggered by the PR_ASSERT at the top of GetSECITem? If so, can you change the comment to PR_ASSERT?
Attachment #8416686 - Flags: review?(mmc) → review+
Comment on attachment 8416686 [details] [diff] [review]
improve-GetSECItem-parameter-validation.patch

Review of attachment 8416686 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixder.h
@@ +182,5 @@
>    class Mark
>    {
>    private:
>      friend class Input;
> +    explicit Mark(const Input& input, const uint8_t* mark)

Removed "explicit "

@@ +196,3 @@
>    {
> +    if (&mark.input != this || mark.mark > input) {
> +      PR_NOT_REACHED("invalid mark");

This is the PR_NOT_REACHED referred to in that comment.

@@ +203,1 @@
>      // TODO: Return false if bounds check fails

Removed the comment since this patch fixes the TODO.
https://hg.mozilla.org/mozilla-central/rev/1cb2ccd1604f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.