HTML input widget/image: check SRC= origin - input src needs CheckLoadURL

VERIFIED DUPLICATE of bug 69070

Status

()

Core
Security
P3
normal
VERIFIED DUPLICATE of bug 69070
19 years ago
12 years ago

People

(Reporter: Norris Boyd, Assigned: Heikki Toivonen (remove -bugzilla when emailing directly))

Tracking

({testcase})

Trunk
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:mustfix])

Attachments

(2 attachments)

(Reporter)

Description

19 years ago
Entering all security bugs and tasks for SeaMonkey into Buzilla for schedule
tracking.
(Reporter)

Updated

19 years ago
Blocks: 7252

Updated

19 years ago
Assignee: kostello → mjudge
Target Milestone: M11

Comment 1

19 years ago
Mike, I'm assigning this to you. The plan for the HTML widget is to use src= to
initialize the widget as opposed to having to embed the HTML. The requirement is
that the URL must be based on the document location.

Comment 2

19 years ago
Mike, do you have any idea what this bug really means?

Comment 3

19 years ago
I'm guessing that this bug is for any input fields we do that have a src.  If I
remember correctly, the src can lead to security risks.  Norris will know for
sure.  Jim Roskind, Mike and I had a discussion about this for the ender html
form widget (textarea type="html") last summer.
(Reporter)

Comment 4

19 years ago
Yes, the restriction is that the src on the input widget can only be for a
document from the same origin as the page came from. Otherwise, a malicious page
could read an arbitrary html file into a html input widget and then submit it
back to the malicious server, effectively stealing private information from the
client.

Updated

19 years ago
Assignee: mjudge → buster

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M11 → M15

Comment 5

19 years ago
HTML edit fields are not a committed feature, so pushing out to M15.

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → REMIND
Target Milestone: M15 → M20

Comment 6

18 years ago
html input widgets will not be supported in the first release.  Setting to M20,
REMIND.

Comment 7

18 years ago
Bulk moving all Browser Security bugs to new Security: General component.  The 
previous Security component for Browser will be deleted.
Component: Security → Security: General

Comment 8

18 years ago
Changing Qa contact to myself.
QA Contact: dshea → junruh

Comment 9

18 years ago
Changing QA to czhang.
QA Contact: junruh → czhang

Comment 10

17 years ago
Reopening and setting to future.
Status: RESOLVED → REOPENED
QA Contact: czhang → junruh
Resolution: REMIND → ---
Target Milestone: M20 → Future

Comment 11

17 years ago
QA > ckritzerh@netscape.com

Comment 12

17 years ago
QA > ckritzer
QA Contact: junruh → ckritzer
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
Status: REOPENED → NEW

Comment 14

16 years ago
no activity on this bug for over a year. I am reassigning it to core owner and 
core qa contact. 
Assignee: attinasi → mstoltz
QA Contact: ckritzer → bsharma
Target Milestone: Future → ---
Need to check and make sure this isn't a problem.
Group: security?
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha

Comment 16

16 years ago
mstoltz, I think saari@netscape.com and mjudge@netscape.com should be Cc'd on
this bug since it is about SRC= on an html area.
Why don't we just close this bug as INVALID since we don't currently support an
HTML input area? (and make it public)
Whiteboard: [roc:invalid?]
Whiteboard: [roc:invalid?] → [sg:invalid?]
I suspect this HTML input widget doesn't support a SRC attribute. sarri, mjudge,
can you comment?
Hmm, I went and checked the IDL, and the input element does actually have a src
attribute, and it is defined in the DOM rec as well. If the type attribute is
image, src points to the image that should be displayed as the control. So I
guess this is similar to bug 134986 but I don't know if the input element fires
onload when image is loaded. A crude way to do this without the image load event
would be to time how long it takes to load the page (different time when image
is there or not).
to the old editor team: does this HTML input widget still exist?
Assignee: mstoltz → heikki
Status: ASSIGNED → NEW

Comment 21

15 years ago
I don't think it ever did exist -- but what IDL did Heikki check re comment 19?
http://lxr.mozilla.org/mozilla/source/dom/public/idl/html/nsIDOMHTMLInputElement.idl
The spec is: http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-97320704
We use the src attribute of the input element only if type="image". When then
try to retrieve the resource, and display it as image. If the resource is not
image or it can not be loaded, we display a weird "Submit Query" button that
does not look like button at all.

I think this bugs is almost the same as bug 134986, with the exception that one
talks about img and the other about the input element. Since the other bug is
public I think this could also be made public. If someone disagrees with this
please comment ASAP.

OS: Windows NT → All
Whiteboard: [sg:invalid?] → [sg:mustfix]
Target Milestone: mozilla1.2alpha → ---
Created attachment 107624 [details]
testcase

Input elements with different type and resources, and img elements for
comparison.

Comment 25

15 years ago
From comment 23:

   We use the src attribute of the input element only if type="image". When then
   try to retrieve the resource, and display it as image. If the resource is not
   image or it can not be loaded, we display a weird "Submit Query" button that
   does not look like button at all.

That weird "Submit Query" is coming from the GetAlternateTextFor method in
layout/html/style/src/nsCSSFrameConstructor.cpp.

I agree that this is the same as bug 134986.  And I believe that the security
aspect of it can be solved if we can force the onload handler to fire when we
encounter the code above.

Comment 26

15 years ago
FWIW, here's the stack trace at the time the weird "Submit Query" text is created.

nsCSSFrameConstructor::GetAlternateTextFor(nsIContent * 0x03ac66a8, nsIAtom *
0x0112ec48, nsString & {""}) line 10788
nsCSSFrameConstructor::ConstructAlternateFrame(nsIPresShell * 0x03aa2438,
nsIPresContext * 0x03c0a0f0, nsIContent * 0x03ac66a8, nsIStyleContext *
0x03ac2220, nsIFrame * 0x03ac20ec, nsIFrame * & 0x00000000) line 10811 + 28 bytes
nsCSSFrameConstructor::CantRenderReplacedElement(nsCSSFrameConstructor * const
0x03aa22e0, nsIPresShell * 0x03aa2438, nsIPresContext * 0x03c0a0f0, nsIFrame *
0x03ac22dc) line 10987 + 42 bytes
StyleSetImpl::CantRenderReplacedElement(StyleSetImpl * const 0x03aa20e8,
nsIPresContext * 0x03c0a0f0, nsIFrame * 0x03ac22dc) line 1617 + 35 bytes
FrameManager::HandlePLEvent(CantRenderReplacedElementEvent * 0x03acb9e8) line 1182
PL_HandleEvent(PLEvent * 0x03acb9e8) line 644 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00c2dfb8) line 574 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x62a9077e, unsigned int 0x0000c174, unsigned int
0x00000000, long 0x00c2dfb8) line 1335 + 9 bytes
USER32! 77e7124c()
00c2dfb8()

Comment 27

15 years ago
OK, here's how to fire the onload handler in the event that the image is not
found.  Towards the end of the OnStopDecode method of nsImageFrame.cpp there is
the following code:

    if (imageFailedToLoad) {
      // Send error event
      FireDOMEvent(NS_IMAGE_ERROR);
    } else if (mCanSendLoadEvent) {
      // Send load event
      mCanSendLoadEvent = PR_FALSE;

      FireDOMEvent(NS_IMAGE_LOAD);
    }

If you factor out the FireDOMEvent(NS_IMAGE_LOAD) statement so that it gets
called even in the error case, the onload handler gets called and the security
implications of this bug report are nullified.

Are there any downsides to calling the onload handler in the failure case?  If
so, perhaps we can restrict this so that the onload handler is called only if
the source of the image is from the same origin that the page came from. 

Comment 28

15 years ago
Created attachment 108333 [details] [diff] [review]
fire onload handler even if image does not load

Comment 29

15 years ago
correction to comment 27:

   perhaps we can restrict this so that the onload handler is called only if
   the source of the image is from the same origin that the page came from. 

That should have been "is NOT from the same origin..."
Comment on attachment 108333 [details] [diff] [review]
fire onload handler even if image does not load

I'm really afraid that this will break sites if we start firing the onload
handler on img's and input type=image even if the image didn't load. IMO doing
that is just plain wrong.

What exactly are we trying to fix here?
Attachment #108333 - Flags: superreview-
We should of course do what the specs say, but I am not sure if they say
anything about this. But it seems wrong to fire the load event if there is no
image/load fails.

What we can do instead, is to add basic security checks for this and img element
so that you can't use this to check local file existence.

Then there is another bug to implement the security zone feature like MS so that
you can protect your intranet sites from this data leak (this is of course a
superset of the previous paragraph).
Opening to the public since this is basically the same issue as bug 134986 which
is also public, and there were no objections to opening this up.
Group: security
So... What is this bug about?  Needing to do security checks on the src of image
inputs?  If so, that's bug 69070...

If not, what are we trying to fix?
Depends on: 134986
*** Bug 69070 has been marked as a duplicate of this bug. ***
Please move dependencies when dupping, ok?   And I'm not sure of the wisdom of
dupping a clear bug with relevant discussion (bug 69070) to this bug (which
covers something no one can quite figure out....)
Blocks: 55237
Depends on: 78831, 166166
The only reason I left this as the real bug is because of the really low bug
number :)
Summary: HTML input widget: check SRC= origin → HTML input widget: check SRC= origin - input src needs CheckLoadURL

Updated

15 years ago
Keywords: testcase
Summary: HTML input widget: check SRC= origin - input src needs CheckLoadURL → HTML input widget/image: check SRC= origin - input src needs CheckLoadURL
Depends on: 69070

*** This bug has been marked as a duplicate of 69070 ***
Status: NEW → RESOLVED
Last Resolved: 18 years ago13 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.