Potentially-unsafe code in VirtualSocketServer::Evaluate

RESOLVED INVALID

Status

()

RESOLVED INVALID
3 years ago
2 years ago

People

(Reporter: q1, Unassigned)

Tracking

({sec-audit, sec-other})

38 Branch
sec-audit, sec-other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows; rv:***) Gecko/20100101 Firefox/**.*
Build ID: 20150305021524

Steps to reproduce:

VirtualSocketServer::Evaluate (media\webrtc\trunk\webrtc\base\virtualsocketserver.cc) might be able to reference a nonexistent element of a std::vector:

1045:  double VirtualSocketServer::Evaluate(Function* f, double x) {
1046:    Function::iterator iter =
1047:        std::lower_bound(f->begin(), f->end(), x, FunctionDomainCmp());
1048:    if (iter == f->begin()) {
1049:      return (*f)[0].second;
1050:    } else if (iter == f->end()) {
1051:      ASSERT(f->size() >= 1);
1052:      return (*f)[f->size() - 1].second;
1053:    } else if (iter->first == x) {
    ...

The problem occurs if an empty vector *f is possible. In this case, lines 1046-47 set iter==f->end(), which also == f->begin() (because f isa std::vector<Point> *). Then line 1049 references the nonexistent element 0 and returns its .second field, possibly causing incorrect and/or insecure operation down the line.

I do not know whether it is possible for *f to be empty when Evaluate is called.
Component: Untriaged → WebRTC
Flags: needinfo?(rjesup)
Keywords: sec-audit
Product: Firefox → Core
I'd suggest reporting this to upstream.  VirtualSocketServer appears to be used only in unit tests in upstream code.  Currently we don't build these unit tests (and we don't build virtualsocketserver.cc) and it's not in any released Firefox executables.

You can report it at https://code.google.com/p/webrtc/issues/list.  Our code is based on the webrtc 40 stable branch.

Likely we should close this bug
Flags: needinfo?(rjesup)
Keywords: sec-other
This is in code we don't compile or use
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
(Reporter)

Comment 3

3 years ago
I suggest making this code uncompilable with a #error directive, so that it gets proper security review if someone decides to use it in the future.
(In reply to q1 from comment #3)
> I suggest making this code uncompilable with a #error directive, so that it
> gets proper security review if someone decides to use it in the future.

That means we have to make and carry more patches to upstream code (and merge them).  And extending that, any ifdefed code we don't compile-configure in would need #errors....  Far better is to just file the bug against the actual source, the upstream project, and if you like add a comment to the moz.build file referencing it.

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.