Closed Bug 1437169 Opened 3 years ago Closed 2 years ago

Improve error checking in parse_fingerprint in attribute_type.rs

Categories

(Core :: WebRTC: Signaling, defect, P3)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dminor, Assigned: johannes.willbold)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

There's minimal error checking in parse_fingerprint, more is needed.
If we don't want to do it in Rust, we could also move the fingerprint error checking in sipcc to a base class and use it in both implementations.
Assignee: nobody → johannes.willbold
Rank: 25
Comment on attachment 8992795 [details]
Bug 1437169: Improved error checking in the the fingerprint parsing,

https://reviewboard.mozilla.org/r/257634/#review265068

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:592
(Diff revision 1)
> +      default:
> +        algorithm = '?';

I am not a fan of using default when switching over an enum, I prefer that we put every possible value in here, so the compiler will complain if something is added to the enum but not to the switch statement.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:941
(Diff revision 1)
>      }
> +
> +    let fingerprint_token = tokens[1].to_string();
> +    let parse_tokens = |expected_len| -> Result<Vec<u8>, SdpParserInternalError>{
> +        let bytes = fingerprint_token.split(":")
> +                                     .map(|byte_token| u8::from_str_radix(byte_token, 16))

I wonder if we should reject stuff like "0000A" and "B".

I assume this already fails for "" and "0xFB"?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:961
(Diff revision 1)
> +        unknown => {
> +            return Err(SdpParserInternalError::Generic(
> +                format!("fingerprint contains unknown hash algorithm '{}'", unknown)
> +            ))
> +        }

We should ignore (and maybe warn) when we encounter a fingerprint type we do not support.
Attachment #8992795 - Flags: review?(docfaraday) → review+
Comment on attachment 8992795 [details]
Bug 1437169: Improved error checking in the the fingerprint parsing,

https://reviewboard.mozilla.org/r/257634/#review265068

> I wonder if we should reject stuff like "0000A" and "B".
> 
> I assume this already fails for "" and "0xFB"?

Currenlty, "" and "0xFB" gets rejected but "0000A" and "B" is accepeted. The standard says they should be rejected as well. I added an extra condition that checks that the byte tokens have exaclty 2 hexdigits. Also, I added these special cases to the testcases.

> We should ignore (and maybe warn) when we encounter a fingerprint type we do not support.

Now, it will ignore the fingerpint line and output a warning about it.
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/aa652865d3ad
Improved error checking in the the fingerprint parsing, r=bwc
https://hg.mozilla.org/mozilla-central/rev/aa652865d3ad
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.