WebSocketImpl::WebSocketImpl does not initialise mSecure

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: jseward, Assigned: baku)

Tracking

({csectype-uninitialized, sec-moderate})

Trunk
mozilla52
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr45 wontfix, firefox50 wontfix, firefox51+ fixed, firefox52- fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
This is dynamically observable in our test suites.  STR to follow.

Setting as sec-sensitive because it seems to me to be ungood to be
confused about whether a connection is secure or not.  Please declassify
as appropriate.
(Reporter)

Comment 1

3 years ago
Posted file Valgrind complaint
(Reporter)

Comment 2

3 years ago
Looking at dom/base/WebSocket.cpp, it seems that class WebSocketImpl
has a |bool mSecure|, but |explicit WebSocketImpl(WebSocket* aWebSocket)|
does not give an initial value for it.  I am hypothesising that this is
the cause of the reported complaint.
Group: core-security → dom-core-security
Flags: needinfo?(amarchesini)
(Assignee)

Updated

3 years ago
Flags: needinfo?(amarchesini)
(Assignee)

Comment 3

3 years ago
Posted patch ws.patchSplinter Review
Attachment #8805213 - Flags: review?(kyle)
Attachment #8805213 - Flags: review?(kyle) → review+
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
(Reporter)

Comment 4

3 years ago
(In reply to Andrea Marchesini [:baku] from comment #3)
> Created attachment 8805213 [details] [diff] [review]
> ws.patch

Verified as fixing the complaint.

The test is devtools/server/tests/mochitest/test_websocket-server.html

FTR, command line I used is

DISPLAY=:1.0 G_SLICE=always-malloc ./mach mochitest -f chrome --debugger=v312BRANCH \
  --debugger-args="--fair-sched=yes --smc-check=all-non-file \
  --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc-may2015.supp --error-limit=no \
  --trace-children=yes \
  --child-silent-after-fork=yes --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java \
  --num-transtab-sectors=24 --tool=memcheck --freelist-vol=300000000 --redzone-size=64 \
  --gen-suppressions=no --px-default=allregs-at-mem-access \
  --px-file-backed=unwindregs-at-mem-access --stats=no --show-mismatched-frees=no \
  --fullpath-after=/MOZ/ --num-callers=20 --track-origins=no" --keep-open=no \
  devtools/server/tests/mochitest/test_websocket-server.html 2>&1 | tee spew-01-06-mc
https://hg.mozilla.org/mozilla-central/rev/b734eaa69060
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tracking 52- since this is now resolved fixed.
Hi :dveditz, 
Since 51 is also affected, is this worth uplifting to 51?
Flags: needinfo?(dveditz)
Track 51+ as sec-moderate.
sure, this is a trivially safe fix to uplift to 51 (and ESR for that matter).
Flags: needinfo?(dveditz)
Hi :baku,
could you please nominate this uplift to Aurora51 & ESR?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8805213 [details] [diff] [review]
ws.patch

Approval Request Comment
[Feature/regressing bug #]: WebSocket ported to webIDL
[User impact if declined]: mSecure is not initialized in the CTOR. In theory until the initialization is completed in other methods, this value is unset.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8805213 - Flags: approval-mozilla-aurora?
Comment on attachment 8805213 [details] [diff] [review]
ws.patch

Fix a sec-moderate. Take it in 51 aurora.
Attachment #8805213 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: dom-core-security → core-security-release
This may as well wait for the ESR that will go out with 51 (45.7.0).
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: core-security-release
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.