Dictionaries inside unions should be optional

RESOLVED FIXED in Firefox 27

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: reuben, Assigned: bzbarsky)

Tracking

({regression, sec-other})

Trunk
mozilla27
regression, sec-other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25 unaffected, firefox26 unaffected, firefox27+ fixed, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 unaffected)

Details

(Whiteboard: [qa-])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 820373 [details] [diff] [review]
Dictionaries in unions should be optional

I wanted to have the following WebIDL:

[Constructor(optional (ContactProperties or mozContact) properties),
 JSImplementation="@mozilla.org/contact;1"]
interface mozContact { }

But then calling it like this:

 var c = new mozContact();

Fails with:

 TypeError: Argument 1 of mozContact.constructor could not be converted to any of: mozContact, ContactProperties.

When it should initialize an empty ContactProperties dictionary like it does if you have [Constructor(optional ContactProperties properties)].

I'm not sure my patch is the right way to fix this. With it, any union with a dictionary in it is automatically optional. This makes sense if you think that dictionaries are always optional, but at the same time it doesn't sound 100% right, because it means marking the union as optional is useless.

Olli, you worked on Bug 918011. What do you think?
Attachment #820373 - Flags: review?(bugs)

Comment 1

5 years ago
Could you please add tests.
I think updating TestInterface interface and its implementation's declaration is enough.
(Reporter)

Updated

5 years ago
No longer blocks: 929287
(Reporter)

Updated

5 years ago
Blocks: 929495
(Reporter)

Comment 2

5 years ago
TestInterface has
void passUnion10(optional (EventInit or long) arg);
(Reporter)

Comment 3

5 years ago
TestInterface has:

 void passUnion10(optional (EventInit or long) arg);

So this bit of codegen is already being exercises, it just doesn't behave correctly.
(Reporter)

Comment 4

5 years ago
Created attachment 820444 [details]
_construct without the patch
(Reporter)

Comment 5

5 years ago
Created attachment 820445 [details]
_construct with the patch

Comment 6

5 years ago
Comment on attachment 820373 [details] [diff] [review]
Dictionaries in unions should be optional

There will be some a bit more complicated patch.
Attachment #820373 - Flags: review?(bugs)
(Assignee)

Comment 7

5 years ago
This is bad.  If you look at the codegen for passUnion10, you get:

  EventInitOrLong arg0;
  EventInitOrLongArgument arg0_holder(arg0);
  {
    bool done = false, failed = false, tryNext;
    if (!done) {
      done = (failed = !arg0_holder.TrySetToEventInit(cx, args[0], args[0], tryNext)) || !tryNext;

which is totally wrong when args.length() == 0!
Group: core-security
(Assignee)

Comment 8

5 years ago
And I don't know how I missed this when reading over the test codegen from bug 918011.  :(
tracking-firefox27: --- → ?
(Assignee)

Comment 9

5 years ago
This is actually a bug in the handling of null default values for unions.  This IDL also tickles it:

  void passUnion13(optional (object or long?) arg = null);

leading to this codegen:

  ObjectOrLongOrNullArgument arg0_holder(arg0);
  if (args[0].isNullOrUndefined()) {
    arg0_holder.SetNull();

which is bogus if args.length() == 0.
And in particular what's buggy is a non-nullable union containing a nullable type, with a default value of null.
Blocks: 918011
status-b2g18: --- → unaffected
status-firefox25: --- → unaffected
status-firefox26: --- → unaffected
status-firefox27: --- → affected
status-firefox-esr17: --- → unaffected
status-firefox-esr24: --- → unaffected
tracking-firefox27: ? → +
No longer depends on: 918011
Flags: needinfo?(bzbarsky)
Keywords: regression
Looks like we do have a couple of instances of this construct in our exposed .webidl (HTML option and select .add() for instance). Dunno if this is currently exploitable though.
Flags: needinfo?(bzbarsky)
> HTML option and select .add() for instance

If you just mean HTMLSelectElement.add(), that is NOT affected by this bug: it has a nullable union, not a union with a nullable member.

I just checked and we have no existing instances of the problematic pattern in our non-test WebIDL.  Not sure whether we want to keep this sec-sensitive, given that.
Another example of test-only badness.  The init code for the (EventInit or long) member of the test Dict dictionary looks like this:


  {
    bool done = false, failed = false, tryNext;
    if (!done) {
      done = (failed = !mEventInitOrLong.TrySetToEventInit(cx, temp.ref(), &temp.ref(), tryNext)) || !tryNext;

but if isNull, then temp is not even constructed, so temp.ref() is garbage.

With the fix I'm about to post, this looks like so instead:

  if (!(!isNull && !temp.ref().isUndefined())) {
    if (!mEventInitOrLong.SetAsEventInit().Init(cx, JS::NullHandleValue, "Member of EventInitOrLong")) {
      return false;
    }
  } else {
    {
      bool done = false, failed = false, tryNext;
      if (!done) {
        done = (failed = !mEventInitOrLong.TrySetToEventInit(cx, temp.ref(), &temp.ref(), tryNext)) || !tryNext;

and the example from comment 9 becomes:

  if (!(args.hasDefined(0)) || args[0].isNullOrUndefined()) {
    arg0_holder.SetNull();
  } else {

while the example from comment 7 becomes:

  if (!(args.hasDefined(0))) {
    if (!arg0.SetAsEventInit().Init(cx, JS::NullHandleValue, "Member of EventInitOrLong")) {
      return false;
    }
  } else {
    {
      bool done = false, failed = false, tryNext;
      if (!done) {
        done = (failed = !arg0_holder.TrySetToEventInit(cx, args[0], args[0], tryNext)) || !tryNext;

so they all seem to be good.  And passUnion11 becomes:

  if (!(args.hasDefined(0))) {
    if (!arg0.SetAsCustomEventInit(cx).Init(cx, JS::NullHandleValue, "Member of CustomEventInitOrLong")) {
      return false;
    }
  } else {

etc (note the "cx" that needs to be passed there).
Created attachment 820761 [details] [diff] [review]
Fix null default values for non-nullable unions
Assignee: nobody → bzbarsky
Attachment #820761 - Flags: review?(dzbarsky)
Attachment #820761 - Flags: review?(bugs)
(In reply to On vacation Oct 12 - Oct 27 from comment #7)
> This is bad.  If you look at the codegen for passUnion10, you get:
Yup, we realized that with reuben and peterv yesterday.
I couldn't find any use of this construct so didn't see harm to keep the bug non-security sensitive.
Comment on attachment 820761 [details] [diff] [review]
Fix null default values for non-nullable unions

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

::: dom/bindings/Codegen.py
@@ +3129,5 @@
> +              isinstance(defaultValue, IDLNullValue)):
> +            assert type.hasDictionaryType
> +            assert defaultValue.type.isDictionary()
> +            if not isMember and typeNeedsRooting(defaultValue.type):
> +                ctorArgs = "cx"

Ah, I didn't know we started requiring this

@@ +3133,5 @@
> +                ctorArgs = "cx"
> +            else:
> +                ctorArgs = ""
> +            initDictionary = (
> +                'if (!%s.SetAs%s(%s).Init(cx, JS::NullHandleValue, "Member of %s")) {\n'

Why not use CGIfWrapper here?

::: dom/bindings/parser/WebIDL.py
@@ +2437,5 @@
>          if type.isUnion():
> +            # We use the flat member types here, because if we have a nullable
> +            # member type, or a nested union, we want the type the value
> +            # actually coerces to, not the nullable or nested union type.
> +            for subtype in type.unroll().flatMemberTypes:

I still think it's ridiculous that we treat (long or Dict?) differently than (long or Dict)?
Attachment #820761 - Flags: review?(dzbarsky) → review+
What sort of rating should this get?  It seems like at best this is just reading uninitialized memory, but maybe we could be writing to uninitialized memory, too?
As far as I see we don't have any (non-test-only) interfaces using the problematic construct.
Keywords: sec-other
In that case, I'll just mark it sec-other.  Thanks for looking into this on your vacation, Boris!
> Ah, I didn't know we started requiring this

That's needed because in that case our member is a RootedDictionary.

> Why not use CGIfWrapper here?

No good reason.  Fixed.

I checked in https://hg.mozilla.org/mozilla-central/rev/36fd56ac71dd with dzbarsky's review in the interests of not missing the merge, but would still appreciate smaug giving this a once-over when he gets the chance.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 820761 [details] [diff] [review]
Fix null default values for non-nullable unions

Generated code looked ok, though
if (!(0 < args.length())) { is a bit ugly.
Attachment #820761 - Flags: review?(bugs) → review+
Is there any landing/uplift remaining here or can we call status-firefox27:fixed ?

Updated

5 years ago
Flags: needinfo?(bzbarsky)
status-firefox27: affected → fixed
Flags: needinfo?(bzbarsky)
Unless there is a test case or reproducible steps, QA is going to assume this can't be easily verified. Feel free to challenge that, though.
I don't think there's anything QA needs to do here, no.
Thank you, Boris.
Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.