Closed Bug 1352082 Opened 3 years ago Closed 3 years ago

undefined behavior in nsHtml5{Attribute,Element}Name::bufToHash due to signed left shifts

Categories

(Core :: DOM: HTML Parser, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: froydnj, Assigned: hsivonen)

Details

(Keywords: sec-audit, Whiteboard: [adv-main55-])

Attachments

(2 files)

nsHtml5ElementName::bufToHash does:

  int32_t hash = len;
  hash <<= 5;
  hash += buf[0] - 0x60;
  int32_t j = len;
  for (int32_t i = 0; i < 4 && j > 0; i++) {
    j--;
    hash <<= 5;
    hash += buf[j] - 0x60;
  }
  return hash;

The AttributeName version does something quite similar.  This is derived from the Java:

        int hash = len;
        hash <<= 5;
        hash += buf[0] - 0x60;
        int j = len;
        for (int i = 0; i < 4 && j > 0; i++) {
            j--;
            hash <<= 5;
            hash += buf[j] - 0x60;
        }
        return hash;

The only problem is that while signed left shift is well-defined in Java, it is subject to restrictions in C++: the result of the shift must be representable in the result type, otherwise the behavior is undefined ([expr.shift]p2).

This is probably not a huge deal, as I think all our compilers do the obvious thing, but tools like UBSan are going to complain about this.  Filing this as a security bug for undefined behavior; it's probably sec-low, though.  Could possibly be opened up?
Group: core-security
Keywords: sec-audit
Henri or William, any thoughts here?
Flags: needinfo?(wchen)
Flags: needinfo?(hsivonen)
Priority: -- → P2
I'll add a new type annotation @Unsigned to turns selected Java ints into uint32_t instead of int32_t.
Assignee: nobody → hsivonen
Flags: needinfo?(wchen)
Flags: needinfo?(hsivonen)
Attachment #8856345 - Flags: review?(wchen)
Comment on attachment 8855739 [details]
Bug 1352082 - Avoid shifting a signed integer left in C++.

https://reviewboard.mozilla.org/r/127652/#review131120
Attachment #8855739 - Flags: review+
Attachment #8856345 - Flags: review?(wchen) → review+
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c8f060510ed
Avoid shifting a signed integer left in C++. r=wchen
https://hg.mozilla.org/projects/htmlparser/rev/c34e143979ab00c244b2c07d67ba7a3c298a56dc
Mozilla bug 1352082 - Avoid shifting a signed integer left in C++. r=wchen.
https://hg.mozilla.org/mozilla-central/rev/6c8f060510ed
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [adv-main55-]
You need to log in before you can comment on or make changes to this bug.