Closed Bug 1246301 Opened 4 years ago Closed 4 years ago

Incorrectly parsed value for update interval in shavar response

Categories

(Toolkit :: Safe Browsing, defect, P3)

44 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mwobensmith, Assigned: dimi)

References

Details

(Whiteboard: tpe-seceng)

Attachments

(1 file, 1 obsolete file)

With the (malformed) shavar response:

n:9!=Ao[JDw+aN'|e}[ ]2


Results:

Firefox indicates an update success, with an update interval of 9 seconds (later corrected to 1800 seconds, since this is too short). The n field should be an integer, but the string is parsed and interpreted as an integer only until it reaches non-numeric characters

Expected:

Reject invalid parameter for n value.


This is not a big issue by itself, just odd behavior.
Blocks: 1149867
Priority: -- → P3
Assignee: francois → nobody
We should treat this as a failed update.
Whiteboard: tpe-seceng
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Attached patch check update interval value (obsolete) — Splinter Review
Hi François,
You mentioned we should treat incorrect update interval as an update failure, but currently in ProtocolParser.cpp when n cannot be parsed successfully, we just set |mUpdateWait| to 0 [1].

I guess the behavior for incorrect value of n(too small or too large) and parse failure should sync, do you think we should return error or just set |mUpdateWait| to 0?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/ProtocolParser.cpp?from=ProtocolParser.cpp#133
Attachment #8732695 - Flags: feedback?(francois)
Comment on attachment 8732695 [details] [diff] [review]
check update interval value

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

If it works, my preference here would be to not touch the mUpdateWait unless the PR_sscanf() worked and then return an error code immediately (i.e. something else than NS_OK), aborting the parsing.

In terms of enforcing a minimum/maximum size, isn't this code sufficient? https://hg.mozilla.org/mozilla-central/rev/4d76f652669d
Attachment #8732695 - Flags: feedback?(francois)
It turns out that the PR_sscanf() doesn't fail when it gets "9!=Ao[JDw+aN'|e}[ ]2", it simply returns "9" which we fix up later. I think that's OK.

The only case that would be worth fixing IMHO is "n:foobar" which will cause PR_sscanf() to return an error. In that case, we should return an error immediately and abort the parsing.
Attachment #8732695 - Attachment is obsolete: true
Comment on attachment 8734599 [details]
MozReview Request: Bug 1246301 - Incorrectly parsed value for update interval in shavar response. r=francois

https://reviewboard.mozilla.org/r/42387/#review39223
Attachment #8734599 - Flags: review?(francois) → review+
https://hg.mozilla.org/mozilla-central/rev/f3160a1e4fb6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.