133170 - XMLHttpRequest() fails when authentication needed and http redirect vulnerability

Status: VERIFIED FIXED

(Core :: XML, defect, P2)

Description

[Michael Gannon]

XMLHttpRequest() currently has no way of specifying a username/password for use 
with a proxy. Maybe something like the solution for bug 122495 
http://bugzilla.mozilla.org/show_bug.cgi?id=122495 is needed.

This following Javascript code fragment, used from a chrome file, crashes with 
a warning:

function test() { 
  // Create a new http request
  var p = new XMLHttpRequest();
  p.open("GET", "http://ww.ireland.com", false);
  p.send(null);

  var d = p.responseText;
  dump("ResponseText: \n" + d + "\n");

  var s = p.status;
  dump("StatusText: \n" + s + "\n");	
	
  var h = p.getAllResponseHeaders();
  dump("ResponseHeaders: \n" + h + "\n");	
  return;
}

The following warning is displayed:
WARNING: notification callbacks should provide nsIAuthPrompt, file 
D:\mozilla\dev3\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp, line 1631

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

You can provide optional user and password parameters in the open() method.

What does IE do with your testcase?

Comment 2

[Michael Gannon]

Attachment: Modified syncget.html so it can be used by IE

Modified version of syncget.html so that it can be used by IE aswell as Moz

Comment 3

[Michael Gannon]

For testing I used the attached testcase.
I am behind a proxy which needs a username and password to get access.
I have set up my browsers not to go through the proxy when accessing localhost

BLANK: This means when I start up the browser with about:blank, so that it
doesn't ask me for my user/pass firstoff.

REMOTE: This means when I start up the browser with a remote webpage, so that it
asks me for user/pass before it will show it.

_IE6_
When starting from BLANK:
When asking for a localhost XML file it works fine.
When asking for a remote XML file, it pops up the dialog to enter user/pass and
then works fine.

When starting from REMOTE:
When asking for a localhost XML file it works fine.
When asking for a remote XML file it works fine - no popup.

_Moz 0.9.9_
When starting from BLANK:
When asking for a localhost XML file it works fine.
When asking for a remote XML file, it CRASHES:
"WARNING: notification callbacks should provide nsIAuthPrompt, file
D:\mozilla\dev3\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp, line 1631
warning: property cp1256 already exists"

When starting from REMOTE:
When asking for a localhost XML file it works fine.
When asking for a remote XML file it works fine - no popup.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, I'll take your word for it. Confirming. We should definitely not crash.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hmm... I don't know how to test this. Any ideas?

Comment 6

[Michael Gannon]

Not really to be honest... but if you send on patches to me, I'll test them 
gladly. I would really like to see a fix for this bug if possible.

Comment 7

[David Barnes]

I am using Build 2002042908 on W2K and not able to make it crash. Instead, I
just see a response saying "you are not authorized" issued by the Proxy. The
same file in IE will prompt me for a username/password to get through the proxy.

For test, you can use an Apache 2 proxy I just set up at home. Configure Mozilla
to use the address of http://algonquin.tzo.com and the port of 8080.

username: test
password: mozilla

After I have authenticated to the proxy, then the file will work as expected
through Mozilla. You know if it is going through the proxy because there will be
a Via response header that looks something like this:
Via: 1.1 www.mozilla.org:8080

Note - my home connection is from a flaky DSL provider. Let me know if I should
change the port number.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Thanks, lowering priority and removing crash keyword.

I think I have an idea how to fix this now. Note to self: Need to have
XMLHttoRequest implement nsIHttpEventSink(?) and interface requestor and in
GetInterface respond to prompt IID. Dunno if who implements the prompt yet.

Comment 9

[Darin Fisher]

you just need to provide a nsIAuthPrompt implementation.  the windowwatcher
service can provide this.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: WIP, does not work

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Even after bug 141061 was fixed it is still possible to read stuff from http
URLs. I'll fix that as well as part of this bug.

Bringing back to 1.0.

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: blocks redirect to other host, brings up auth prompt

Ok, this seems to fix the security issue with XMLHttpRequest and redirects. It
also brings up the auth prompt, but my first test seemed to indicate the
connection stopped there. I need to do more testing.

Darin, how do I set up Apache to ask for username & password in some directory?
I think there was some .file that I could place in the directory, but I can't
remember (and not sure if I need to do something to enable that first).

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Mitch, how does this look?

Comment 14

[Rakesh Mishra]

changing QA contact

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, I confirmed that the prompt also works.

To test redirect, go to http://sec.greymagic.com/adv/gm001-ns/ and put
http://slashdot.org/users.pl?op=edituser as the demo URL, and hit Snif. You
should not get any results back (and you should get security error in JS console).

To test auth prompt, edit the get.html test in
mozilla/extensions/xmlextras/tests to point to http://green/heikki/login/ When
you open get.html you should get auth prompt, and if you answer "mozilla"  to
both user & password the prompt should go away and you should get the responseText.

Comment 16

[Darin Fisher]

heikki:

you need to create a .htaccess file in the directory you want to protect:

here's a sample:

  AuthType Basic
  AuthName "basic-realm1"
  AuthUserFile /etc/httpd/passwd/basic-realm1
  Require valid-user

use the command htpasswd to create and populate the password file.

you probably also have to tweak your httpd.conf file to allow the use of
.htaccess files.  for example:

make sure the directory your interested has "AllowOverride AuthConfig" set.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Here are some more tests:

1) XMLHttpRequest opens a connection to same host (green in this case), which
redirects it to mozilla.org

http://green.nscp.aoltw.net/heikki/133170/xmlhttp.html

2) Auth test, give "mozilla" as login & password

http://green.nscp.aoltw.net/heikki/133170/xmlhttpauth.html

3) document.load() with redirect

http://green.nscp.aoltw.net/heikki/133170/load.html

4) document.load() with auth

http://green.nscp.aoltw.net/heikki/133170/loadauth.html

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Add auth prompt for document.load

This patch also contains redirect check for document.load(), but for some
reason it passes the CheckConnect() call (it did not matter if I gave it |cx|
or |nsnull| as first param). I don't know why that does not work here; it works
fine for XMLHttpRequest.

If you try http://green.nscp.aoltw.net/heikki/133170/loadmozilla.html you will
see that the same check in document.load() blocks the call, so I see no reason
why it shouldn't work in redirect.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Log in to Bugzilla and W3C member only pages. Then try the following URLs. You
will notice this allows the attacker access to information he should not have.
This is possible because we save the basic auth information (W3C test) & cookies
that store login (Bugzilla test), so the attacker can use your identity to get
access. These will both naturally be fixed by disabling the redirect, as the
patch he will do.

http://green.nscp.aoltw.net/heikki/133170/xmlhttpbugzilla.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttpw3cauth.html

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Also please note that although the authentication prompt and the security fix
aren't directly related, adding the auth prompt can improve security by alerting
the user to the fact that something is trying to log him into a site he was not
trying to get into (the prompt displays URL & group on server it is trying to
log into). This prevents a hacker from using a timer based get (for example try
to get page every 5 minutes until succeed) on a page that uses basic
authentication, because the user would be peppered by these dialogs. Without
that part of the fix those calls would just silently fail if auth information
was not available. IE6 obviously supports that auth dialog as well.

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Michael, we still have the issue with http redirecting to another http URL,
which will be fixed by the patch here.

Comment 22

[Asa Dotzler [:asa]]

Need this for RC2. 

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Bummer, Mitch realized that the approach I have does not work for asynchronous
case (document.load is async.). He has an idea how to make it work, though. Here
is a test for asynchronous XMLHttpRequest:

http://green.nscp.aoltw.net/heikki/133170/xmlhttpasync.html

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ray, it looks like we will need to change the security checks for XMLHttpRequest
a little to fix this problem. We shouldn't affect SOAP, but I wanted to let you
know just in case...

Comment 25

[Mitchell Stoltz (not reading bugmail)]

Attachment: New patch for Moz 1.0 branch, fixes async case

This patch was done against the 1.0 branch, but it should apply to the trunk as
well. It includes Heikki's earlier patch but adds a new security check function
which correctly handles both synchronous and asnchronous cases. This still
needs testing - we should test cross-site access with privileges, synchronous
and asynchronous.

r=mstoltz on Heikki's part of the patch, Heikki, can you review the rest?

Comment 26

[Mitchell Stoltz (not reading bugmail)]

Attachment: Equivalent patch for NS6.2.1 branch

The same patch, ported to the 6.2.1 branch. Appears to pass Heikki's testcases
in comment 17.

Comment 27

[John Bandhauer]

Comment on attachment 82755 [details] [diff] [review]
Equivalent patch for NS6.2.1 branch

+    if (!sourcePrincipal ||
+	 NS_SUCCEEDED(sourcePrincipal->Equals(mSystemPrincipal, &equals))
+		      && equals)

I'd like to see some grouping parens to make the intent clearer.

This stuff looks reasonable to me. I'd like to see some r= from 
qualified reviewers before this goes in. I'd also like some
assurance that the tests cover the range of cases that might
include possible regressions - redirects that suddenly fail
or unnecessary re-auth dialogs where they had not previously 
been, etc.

Given that, sr=jband

Comment 28

[lchiang]

heikki - if you need add'l tests, pls email [e-mail].  Due to
some time constraints and people working different hours, I want to ensure that
you can get the widest range of help in addition to the Mozilla community
helping.  Thanks.

Comment 29

[chris hofmann]

Comment on attachment 82728 [details] [diff] [review]
New patch for Moz 1.0 branch, fixes async case

a=chofmann for the 1.0 branch.

Comment 30

[John Bandhauer]

Comment on attachment 82728 [details] [diff] [review]
New patch for Moz 1.0 branch, fixes async case

sr=jband With the same comments as on the other patch.

Comment 31

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 82728 [details] [diff] [review]
New patch for Moz 1.0 branch, fixes async case

I second jband's plea to add some parenthesis to make the grouping clear in
nsScriptSecurityManager::CheckSameOrigin().

There is also a small bloat issue: make mCrossSiteAccessEnabled be of type
PRPackedBool, and move it next to the other packed booleans.

r=heikki

Comment 32

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 82755 [details] [diff] [review]
Equivalent patch for NS6.2.1 branch

With same nits r=heikki

Btw, I should note that caps is strange territory for me, but what I
undrerstood seemed good.

Comment 33

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Tests to run:

See comments #17, #18, #19, #23 (or just go to
http://green.nscp.aoltw.net/heikki/133170/ and try them all).

As far as regression tests go:

Online tests are at: http://www.mozilla.org/xmlextras/tests.html

Additional tests at extensions/xmlextras/tests/ (try all the post tests, SOAP
tests, and if you get any SOAP tests from the Evangelism team/Ray Whitmer).
There are also some tests in content/xml/tests/load/.

Due to personal circumstances I can not do much to help right now, but Mitch
should be able to finish the checkins etc. 

Comment 34

[scottputterman]

adding adt1.0.0+.  Mitch, could you check this into the branch tonight?

Comment 35

[selmer (gone)]

Bonsai shows a checkin from Mitch 5/7 23:27, updating keyword foo.

Comment 36

[Heikki Toivonen (remove -bugzilla when emailing directly)]

These should not work after the fix:

http://green.nscp.aoltw.net/heikki/133170/load.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttp.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttpasync.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttpbugzilla.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttpw3cauth.html

These should work:

http://green.nscp.aoltw.net/heikki/133170/loadauth.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttpauth.html

Should not work before or after:

http://green.nscp.aoltw.net/heikki/133170/loadmozilla.html

I think this one is bogus (no correct redirect URL in the CGI):

http://green.nscp.aoltw.net/heikki/133170/xmlhttptc.html

Comment 37

[Rakesh Mishra]

Trying to verify on the branch build 2002-05-08-08-1.0.0. As per comment# 15
,going to http://sec.greymagic.com/adv/gm001-ns/ and putting
http://slashdot.org/users.pl?op=edituser as demo URL and hitting Sniff. 
I get as result "File not found" and I do not get any security error in th JS
console.Is this how it should behave .

Comment 38

[Rakesh Mishra]

verified on the branch build 2002-05-08-08-1.0.0 on Windows XP , with the URLs
under comment #36 . Adding the keyword verified 1.0.0.

Comment 39

[chris hofmann]

did this hit the trunk yet?  let's get it there and mark this fixed.
thanks

Comment 40

[Rakesh Mishra]

Verified on the 6.2.3 branch build 2002-05-08-14-6.2.3 on Windows 2000 , with
the test cases under comment # 36.
Work fine.

Comment 41

[Fernando Cassia]

Any reason for the lack of approval of the 6.2.1 base patch? It's been a few
days.

I'm not pushing anyone, just a concerned Netscape 6.2.2 user waiting for a
patch. :)

Comment 42

[David Barnes]

Could this comment about documen.load() be related? He says it no longer passes 
cookies. http://bugzilla.mozilla.org/show_bug.cgi?id=136891#c21

Comment 43

[Mitchell Stoltz (not reading bugmail)]

Fernando: A patch to 6.2.2 is forthcoming. "Has-Approval" is irrelevant; that
only applies to Mozilla. If you're concerned in the meantime, disable
JavaScript, at least when you visit any sites you don't trust.

Comment 44

[Mitchell Stoltz (not reading bugmail)]

Attachment: New patch for the trunk (the caps part)

OK, here's the patch for the trunk with some changes in caps. I have refactored
code to avoid doing unnecessary codebase principal creation, and to generate
better exceptions / console messages with localization. This will be checked in
along with the earlier 1.0 branch patch in this bug, and the patches from bug
113351 and bug 147754.

Comment 45

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 86700 [details] [diff] [review]
New patch for the trunk (the caps part)

>Index: caps/idl/nsIScriptSecurityManager.idl
>===================================================================
>Index: caps/include/nsScriptSecurityManager.h
>===================================================================
>+    SecurityCompareURIs(nsIURI* aSourceURI,
>+                        nsIURI* aTargetURI,
>+                        PRBool* result);
>+

This name is not very descriptive. CompareProtocolHostPort() would
describe what it does although the name is not "pretty".

>Index: caps/src/caps.properties
>===================================================================
>+CheckLoadURIError = The link to %s was blocked by the security manager. Remote content may not link to local content.

I don't think it is necessarily local content, for example if you have a link
pointing to imap://...?

>Index: caps/src/nsCertificatePrincipal.cpp
>===================================================================
>Index: caps/src/nsCodebasePrincipal.cpp
>===================================================================
> nsCodebasePrincipal::Equals(nsIPrincipal *other, PRBool *result)

In general the style is to have parameters start with 'a'.

>Index: caps/src/nsScriptSecurityManager.cpp
>===================================================================
>+nsScriptSecurityManager::SecurityCompareURIs(nsIURI* aSourceURI,
>+                                             nsIURI* aTargetURI,
>+                                             PRBool* result)
>+{
>+    nsresult rv;

Move rv down to where you use it for the first time.

It would also be a good idea to set result to false in the beginning.

>+    if (aSourceURI == aTargetURI)
>+	{

Indentation...

>+    if (aTargetURI == nsnull) 
>+        // return false
>+        return NS_OK;

In general this is dangerous, because it is easy to later change this
code so that it would require braces without noticing that.

>+    if (!sourceBaseURI || !targetBaseURI)
>+        return NS_ERROR_FAILURE;

You did not set result. Would be fixed by initializing it in the beginning.

>+            *result = targetSpec.Equals(sourceSpec);
>+        } 
>+		else
>+		{

Indentation seems weird again. Also you could consider changing the
whole file to 2 space indents (would help reading code like this
in hear in Emacs etc.)

I didn't have time to read the rest carefully, so just some brief observations:

* ReportError second argument is PRUnichar, check if nsAString would work.
* ReportError checks the localization string format for correct number of
  %-characters. I think this is the only code that does that. How about
  making some new functions into string bundle, or modifying the existing
  methods, to do these checks? That way everyone could get the benefits of
  this work.

Comment 46

[Mitchell Stoltz (not reading bugmail)]

Attachment: caps patch for the trunk v2

OK, I addressed some of Heikki's concerns and also changed the return values
from CheckSameOrigin and CheckSameOriginURI - I was forgetting to return an
error when the check failed (whoops).
- I want to keep the name of SecurityCompareURIs as it is, because there's no
reason callers need to know the exact nature of the check (that it compares
scheme, host, and port). It's a URL comparison for security, that's what's
important.
- Style point: I like to declare |nsresult rv| at the top of a function so that
I don't have to move the declaration around every time I use rv in a different
place.
- Style point: I like 4-space indents better, and this whole module is written
that way, so I don't want to change it now. I did weed out a couple of hard
tabs that were still lurking in the file.
- The second parameter of ReportError could be nsAString, but that would be
harder to use, since I need to pass it to a function that takes PRUnichar*, and
I don't think you can do a .get() on an nsAString. I think the current setup is
simpler, even though it doesn't use the nsString classes.

Comment 47

[Johnny Stenback (:jst)]

Comment on attachment 86898 [details] [diff] [review]
caps patch for the trunk v2

+nsCodebasePrincipal::Equals(nsIPrincipal *aOther, PRBool *result)
 {
-    // Equals is defined as object equality or same origin
+    nsresult rv;
     *result = PR_FALSE;
-    if (this == other) 
-	{
-	 *result = PR_TRUE;
-	 return NS_OK;
-    }
...

Are you sure you want to loose this fast path optimization, I know you're doing
a similar optimization further down when you have the URI's, how commonly would
this case arise? I would suggest leaving it there, but move it above the
assignment of PR_FALSE into *result. And I don't see rv being needed here, the
one place where it's used you can just do a nsCOMPtr null check.

- In nsScriptSecurityManager::SecurityCompareURIs(), are we guaranteed that all
URI's that are ever passed to this method are normalized so that the protocol
is lower case n' so on?

- Nit of the day:

+		 if(!*result && (sourcePort == -1 || targetPort == -1))

Add a space after the "if" for consistency with the rest of the code.

- In nsScriptSecurityManager::ReportError():

+    while (p != end)
+    {
+	 if (*p == '%')
+	 {
+	     ++p;
+	     if (foundPercent || *p != 's')

Accessing *p here w/o checking if p != end after the increment is not safe.

Also:

+    PRUnichar* message = 
+	 nsTextFormatter::smprintf(msgText, spec.get());
...
+	 NS_ENSURE_TRUE(console, NS_ERROR_FAILURE);
...
+    nsTextFormatter::smprintf_free(message);

The NS_ENSURE_TRUE(...) would cause |message| to be leaked. There's also a leak
inside the #ifdef DEBUG block, which doesn't really matter, but should still be
fixed...

Attach a new patch with those issues and I'll have one last look...

Comment 48

[Mitchell Stoltz (not reading bugmail)]

Attachment: Caps patch for the trunk v3

OK, I've addressed those issues.

Comment 49

[Johnny Stenback (:jst)]

Comment on attachment 87146 [details] [diff] [review]
Caps patch for the trunk v3

+	     ++p;
+	     if ((p != end) && (foundPercent || *p != 's'))
+	     {
+		 NS_ERROR("Malformed format string in error message
localization");
+		 return NS_ERROR_FAILURE;
+	     }

Actually, don't you want that to be:

  if (p == end || foundPercent || *p != s)

This way you'll catch invalid strings that end with a '%' character.

sr=jst if you make the above change, assuming you agree with the above.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> +/* Static function for comparing two URLs - for security purposes,
> + * two URLs are equivalent if their scheme, host, and port are equal.

"URIs" instead of "URLs"?

> +   if (NS_SUCCEEDED(rv) && targetScheme.EqualsIgnoreCase(sourceScheme.get())) 

targetScheme.Equals(sourceScheme, nsCaseInsensitiveCStringComparator()),
please.  EqualsIgnoreCase is supposed to go away, iirc.

> +        if (targetScheme.Equals("file"))

Should that not be a case-insensitive comparison?

> +        else if (targetScheme.EqualsIgnoreCase("imap")    ||
> +                 targetScheme.EqualsIgnoreCase("mailbox") ||
> +                 targetScheme.EqualsIgnoreCase("news"))

This may benefit from NS_LITERAL_CSTRING and using a comparator (though the perf
difference is likely not very big, if any).

> +            *result = targetSpec.Equals(sourceSpec);

Are mail URIs case-sensitive?

> + *result = NS_SUCCEEDED(rv) && targetHost.EqualsIgnoreCase(sourceHost.get());

Again, probably better to use a comparator.

> +        rv = ioService->GetProtocolHandler(sourceScheme.get(),
> +                                           getter_AddRefs(protocolHandler));
> +        if (NS_FAILED(rv))
> +        return rv;

Do we really want to fail in this case?  Or return a PR_FALSE for the check and
succeed?

> +    PRBool equals = PR_FALSE;

How about naming this "systemPrincipal" or something descriptive like that?

> +ReportError(cx, NS_LITERAL_STRING("CheckSameOriginError").get(), aTargetURI);

It would be nice to report the sourceURI here as well.....

> +    rv = bundle->GetStringFromName(messageTag, getter_Copies(msgText));

Is there a reason not to use FormatStringFromName here?

Comment 51

[Mitchell Stoltz (not reading bugmail)]

Attachment: Caps patch for the trunk v4

Thanks Boris, I took your suggestions. Please give me your final review ASAP.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 87428 [details] [diff] [review]
Caps patch for the trunk v4

> +    if (aSourceURI == aTargetURI)
> +	{
> +        *result = PR_TRUE;

The indentation on the "{" looks like a tab...

> +    PRBool equals = PR_FALSE;
> +    rv = sourcePrincipal->Equals(mSystemPrincipal, &equals);

Again, that bool could have a better name like |isSystemPrincipal|.

> +const PRUnichar *formatStrings[] = { NS_ConvertASCIItoUCS2(sourceSpec).get(),
> +                                     NS_ConvertASCIItoUCS2(targetSpec).get() };

This is no good.  You're saving pointers to temporaries that go away right
after this line.  You want to do something like:

ucsSourceSpec NS_ConvertASCIItoUCS2(sourceSpec);
ucsTargetSpec NS_ConvertASCIItoUCS2(targetSpec);
const PRUnichar *formatStrings[] = { ucsSourceSpec.get(), ucsTargetSpec.get()
};

Fix those issues, and r=bzbarsky.

Comment 53

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 87428 [details] [diff] [review]
Caps patch for the trunk v4

OK, carrying over r and sr

Comment 54

[Mitchell Stoltz (not reading bugmail)]

Fix checked in on the trunk.

Comment 55

[Rakesh Mishra]

Verified fixed on the trunk build 2002-12-11-08-trunk on Windows 2000 , with
the test cases under comment # 36.

marking verified