Bug 188410 – InputType should not interact with an HTMLInputElement is no longer associated with

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

InputType does not need to be RefCounted and probably shouldn't be.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #0)
> InputType does not need to be RefCounted and probably shouldn't be.

There are cases in which InputType needs to outlive the element itself. We can talk about it offline.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #1)
> (In reply to Chris Dumez from comment #0)
> > InputType does not need to be RefCounted and probably shouldn't be.
> 
> There are cases in which InputType needs to outlive the element itself. We
> can talk about it offline.

InputType was not refcounted until recently so I find it surprising. My patch also does not cause any failures.

Will upload the patch in a bit so we can discuss.

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> (In reply to Ryosuke Niwa from comment #1)
> > (In reply to Chris Dumez from comment #0)
> > > InputType does not need to be RefCounted and probably shouldn't be.
> > 
> > There are cases in which InputType needs to outlive the element itself. We
> > can talk about it offline.
> 
> InputType was not refcounted until recently so I find it surprising. My
> patch also does not cause any failures.
> 
> Will upload the patch in a bit so we can discuss.

And to be clear I am aware of why it was made refcounted in the first place but have a different proposal to deal with the issue.

[Chris Dumez]

Created attachment 346785 [details]
Patch

[EWS Watchlist]

Attachment 346785 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 346785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346785&action=review

r- because of this will re-introduce the bug making InputType RefCounted fixed.

> Source/WebCore/ChangeLog:29
> +        1. Use a EventQueueScope to dealy the firing of events, which we already do in a few places

We can't do this for the particular use-after-free at hand.
We discussed this possibility in length and concluded that it's not viable.

[Ryosuke Niwa]

If you want to re-open this issue, please talk to me in person before doing so. There are some details I can talk about outside the security component.

[Ryosuke Niwa]

The fundamental problem is that EventQueueScope is really broken. I initially introduced it to workaround the most egregious security bugs in editing and a few other places but it probably should never have existed. It was probably one giant mistake.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #8)
> The fundamental problem is that EventQueueScope is really broken. I
> initially introduced it to workaround the most egregious security bugs in
> editing and a few other places but it probably should never have existed. It
> was probably one giant mistake.

Did you look at the patch? I am NOT using EventQueueScope. I am using weakThis check. I looked at the original radar and made sure my patch addresses that crash.

Also, as explained in my change log the current code is wrong. Even though the lifetime  of the InputType got extended, it is does not get detached from its Element when the element changes input type. Therefore, after you change the input's type, the old InputType can keep messing with an HTMLInputElement it is no longer associated with.

Reopening since this issue (mentioned in my change log) needs to be addressed, one way or another.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Ryosuke Niwa from comment #8)
> > The fundamental problem is that EventQueueScope is really broken. I
> > initially introduced it to workaround the most egregious security bugs in
> > editing and a few other places but it probably should never have existed. It
> > was probably one giant mistake.
> 
> Did you look at the patch? I am NOT using EventQueueScope. I am using
> weakThis check. I looked at the original radar and made sure my patch
> addresses that crash.
> 
> Also, as explained in my change log the current code is wrong. Even though
> the lifetime  of the InputType got extended, it is does not get detached
> from its Element when the element changes input type. Therefore, after you
> change the input's type, the old InputType can keep messing with an
> HTMLInputElement it is no longer associated with.
> 
> Reopening since this issue (mentioned in my change log) needs to be
> addressed, one way or another.

Also note that I did discuss my proposal with Brent before implementing this.

[Ryosuke Niwa]

Okay. Then did you run Speedoemter, Dromaeo, & PLT with this change?

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #11)
> Okay. Then did you run Speedoemter, Dromaeo, & PLT with this change?

If people agree with the direction, I would be happy to run those benchmarks. You kinda forced me to upload a patch early to discuss by closing my bug before it even had a patch.

[Ryosuke Niwa]

If the issue we're trying to address is that we're not clearing the element reference in InputType, then that should be clearly stated in the bug title & description.

There is another approach to this problem which is to make InputType store WeakPtr to HTMLInputElement, or clear the pointer when InputType gets disassociated from Element.

[Ryosuke Niwa]

Having said that now that I understand what you're trying to do, I think either approach is fine.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #13)
> If the issue we're trying to address is that we're not clearing the element
> reference in InputType, then that should be clearly stated in the bug title
> & description.
> 
> There is another approach to this problem which is to make InputType store
> WeakPtr to HTMLInputElement, or clear the pointer when InputType gets
> disassociated from Element.

Actually, Brent already landed a follow-up change to have InputType store a WeakPtr to its HTMLInputElement instead of a C++ reference, in order to address new crashes. While it fixed those crashes, it does not exactly address the issue I mentioned, since an HTMLInputElement can stay alive but be associated to a new InputType (when its type attribute is changed).

However, nulling out the m_element pointer when the HTMLInputElement is no longer associated with the InputType would address the issue I mentioned. I thought about this. The reason I did not go with this approach is that it is not clear when you need to null check m_element or not. Right now, most places do not null check m_element (even though it is weak), they merely assert it is not null.

[Ryosuke Niwa]

There is one drawback with this approach, which is that now every member function of InputType MUST have a local Ref/RefPtr to the element since otherwise "this" object can be deleted if it involves any function which may execute scripts.

And after invoking any function which may execute script, it must check that the input type associated with the retained element didn't change its type (otherwise, we'd have a type confusion bug).

[Ryosuke Niwa]

Comment on attachment 346785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346785&action=review

> Source/WebCore/html/CheckboxInputType.cpp:88
> +        element().setIndeterminate(state.indeterminate);
> +        element().setChecked(state.checked);

This code for example should have ScriptDisallowedScope::InMainThread or have a Ref/RefPtr to Element
since setIndeterminate is a non-trivial function which can be modified in the future to dispatch an event.

[Ryosuke Niwa]

My recommendation still is to keep InputType ref-counted and clear the element pointer.

Many WebKit engineers know how to use Ref/RefPtr correctly in most cases. In my experience, whenever we introduced a complicated lifecycle management like one object owning another via unique_ptr, etc... someone eventually comes around and introduce a new security bug.

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> (In reply to Ryosuke Niwa from comment #11)
> > Okay. Then did you run Speedoemter, Dromaeo, & PLT with this change?
> 
> If people agree with the direction, I would be happy to run those
> benchmarks. You kinda forced me to upload a patch early to discuss by
> closing my bug before it even had a patch.

I triggered A/B analysis tasks for these benchmarks.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #18)
> My recommendation still is to keep InputType ref-counted and clear the
> element pointer.
> 
> Many WebKit engineers know how to use Ref/RefPtr correctly in most cases. In
> my experience, whenever we introduced a complicated lifecycle management
> like one object owning another via unique_ptr, etc... someone eventually
> comes around and introduce a new security bug.

The whole reason I started this is because of https://bugs.webkit.org/show_bug.cgi?id=186321. How are people supposed to know when to null-check element or not?

As I mentioned on that bug, we could go the direction you suggest but then we'd likely want to null check element everywhere? This could likely have a perf impact though.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #17)
> Comment on attachment 346785 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346785&action=review
> 
> > Source/WebCore/html/CheckboxInputType.cpp:88
> > +        element().setIndeterminate(state.indeterminate);
> > +        element().setChecked(state.checked);
> 
> This code for example should have ScriptDisallowedScope::InMainThread or
> have a Ref/RefPtr to Element
> since setIndeterminate is a non-trivial function which can be modified in
> the future to dispatch an event.

Sure we could add ScriptDisallowedScope::InMainThread in more places.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #16)
> There is one drawback with this approach, which is that now every member
> function of InputType MUST have a local Ref/RefPtr to the element since
> otherwise "this" object can be deleted if it involves any function which may
> execute scripts.

This is inaccurate, you do not need to refcount your element. As a matter of fact you likely should not. If you ref the element and use your local variable then you still have the issue that you may interact with an element that is no longer associated with you.

The way I suggest and use in my patch is to use a weakThis variable and return early if it becomes null as a result of calling script.

> 
> And after invoking any function which may execute script, it must check that
> the input type associated with the retained element didn't change its type
> (otherwise, we'd have a type confusion bug).

Commented above.

[Chris Dumez]

Created attachment 346867 [details]
Patch

[Chris Dumez]

Created attachment 346869 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346869 [details]
Patch

Clearing flags on attachment: 346869

Committed r234744: <https://trac.webkit.org/changeset/234744>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43117754>

[Brent Fulgham]

*** Bug 186321 has been marked as a duplicate of this bug. ***