Closed Bug 1000354 Opened 6 years ago Closed 6 years ago

Input.Init succeeds with non-null ptr, zero length

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mmc, Assigned: mmc)

Details

Attachments

(1 file)

Brian asked me to add another test case to the pkix unittest, and right now this is passing:

  // Is this a bug?
  ASSERT_EQ(Success, input.Init((const uint8_t*) "hello", 0));

I'm not sure if this is working as intended, but it looks like the relevant code just doesn't check if length is zero.
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #0)
> Brian asked me to add another test case to the pkix unittest, and right now
> this is passing:
> 
>   // Is this a bug?
>   ASSERT_EQ(Success, input.Init((const uint8_t*) "hello", 0));
> 
> I'm not sure if this is working as intended, but it looks like the relevant
> code just doesn't check if length is zero.

I think that this test should pass as written. It just means that input.AtEnd() is initially true, which is harmless. The input class is primarily intended to protect the rest of mozilla::pkix against buffer overflows and there's no risk of buffer overflow with the current behavior. So, I think RESOLVED INVALID is a good resolution. Thanks for adding the test case.
We should update the comment in the test with why we think it's not a bug. Monica - do you have time/interest to do this?
Flags: needinfo?(mmc)
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8418904 - Flags: review?(dkeeler)
Comment on attachment 8418904 [details] [diff] [review]
Fix comment and make test clearer (

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

LGTM.

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +90,5 @@
>    ASSERT_EQ(Failure, input.Init(nullptr, 100));
>    ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
>  
> +  // Though it seems odd to initialize with zero-length and non-null ptr, this
> +  // is working as intended. The input class was intended to protect against

nit:capitalize Input
Attachment #8418904 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/52f249e906ce
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.