Security: flash.sampler.getLexicalScopes() and getSavedThis() should fail if app is not trusted

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P2
blocker
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Mike Morearty, Assigned: Mike Morearty)

Tracking

unspecified
flash10.1
Bug Flags:
flashplayer-qrb +

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.30 KB, patch
Tommy Reilly
: review+
Edwin Smith
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
flash.sampler.getLexicalScopes() and getSavedThis() expose Flash player internals that may be a security risk; for example, getLexicalScopes() returns an array that may include activation objects that are not exposed to the app in any other way.  I'm not sure if getSavedThis() exposes anything dangerous, so it may not be strictly necessary to protect this function, but I would be inclined to protect it.

So, in the C++ code, SamplerScript::getLexicalScopes() and SamplerScript::getSavedThis() should both call trusted(), and should fail (return null or undefined) if trusted() returns false, just as most other functions in SamplerScript do:

For getLexicalScopes():

	AvmCore* core = self->core();
	Sampler *s = core->get_sampler();
	if (!s || !s->sampling() || s->sampleCount() == 0 || !trusted(self))
		return undefinedAtom;

For getSavedThis(), change "return undefinedAtom" to "return NULL".
(Assignee)

Comment 1

8 years ago
Created attachment 426631 [details] [diff] [review]
Proposed patch
(Assignee)

Updated

8 years ago
Attachment #426631 - Flags: superreview?(edwsmith)

Updated

8 years ago
Attachment #426631 - Flags: superreview?(edwsmith) → superreview+

Comment 2

8 years ago
> For getSavedThis(), change "return undefinedAtom" to "return NULL".

The patch doesn't do this and I think it would be wrong since the decl is to return Object and that means Atom in C++.
(Assignee)

Comment 3

8 years ago
Sorry, I got it backwards in my comment -- what I meant was, getSavedThis() should return undefinedAtom if permission is denied, and getLexicalScopes() should return NULL if permission is denied.

Comment 4

8 years ago
Aren't these calls new to Argo? If so, we can land 'em directly. If not, we'll have to land this in tr-sec...
(Assignee)

Comment 5

8 years ago
Yes, these calls are new to Argo.

Comment 6

8 years ago
does this even need to be a security bug?
(Assignee)

Comment 7

8 years ago
Created attachment 426706 [details] [diff] [review]
Revised patch

Revised based on feedback from Tommy: The functions still succeed even if sampling is not currently taking place.
Attachment #426631 - Attachment is obsolete: true
Attachment #426706 - Flags: superreview?(edwsmith)
Attachment #426706 - Flags: review?(treilly)

Updated

8 years ago
Attachment #426706 - Flags: review?(treilly) → review+
(Assignee)

Comment 8

8 years ago
You're right Tommy, I don't think this needs to be a security bug, since it's only in TR and is new to Argo.  Sorry about that.  (I don't have permission to change it because I'm not in the right group)

Comment 9

8 years ago
Comment on attachment 426706 [details] [diff] [review]
Revised patch

rubber stamp
Attachment #426706 - Flags: superreview?(edwsmith) → superreview+

Updated

8 years ago
Assignee: nobody → mmoreart
Group: tamarin-security
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1

Comment 10

8 years ago
tamarin-redux: changeset:   3849:df1838d8ebf3
tamarin-redux-argo:  3721:df1838d8ebf3
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.