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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: improve-GetSECItem-parameter-validation.patch

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

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?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb2ccd1604f

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1cb2ccd1604f