Closed Bug 738396 Opened 8 years ago Closed 7 years ago

Java applets may read arbitrary files on a user's system

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox16 - wontfix
firefox17 + wontfix
firefox18 + wontfix
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 + wontfix
firefox22 + wontfix
firefox23 + verified
firefox24 + verified
firefox25 --- verified
firefox-esr10 --- wontfix
firefox-esr17 23+ verified
b2g18 --- unaffected
b2g-v1.1hd --- unaffected

People

(Reporter: johns, Assigned: johns)

References

Details

(Keywords: sec-high, Whiteboard: [sg:high][adv-main23+][adv-esr1708+])

Attachments

(5 files, 5 obsolete files)

11.21 KB, patch
benjamin
: review+
abillings
: sec-approval+
johns
: checkin+
Details | Diff | Splinter Review
5.96 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
9.63 KB, patch
johns
: review+
abillings
: sec-approval+
johns
: checkin+
Details | Diff | Splinter Review
1.95 KB, application/zip
Details
7.06 KB, patch
johns
: review+
johns
: checkin+
Details | Diff | Splinter Review
Investigating 736965 which is fallout from a related security issue 406541, I discovered the fixes in 406541 are woefully inadequate. There are numerous ways to invoke java plugins which bypass various security checks. I found at least three different issues in the loading code. All of the below issues apply to 3.6+ and probably older.

The most severe of these is <param> elements can override the <object> tag's codebase as seen by java. That means this will load from a non-file: context

>  <applet width=500 height=500 codebase="." code="home.user.some.folder.object.class">
>    <param name="codebase" value="file:///">
>  </applet>

The security check will assume codebase "." is valid, but java will use the file:/// codebase, allowing read access to the full filesystem. There is no prompt when this applet loads.

For this to be successful, you must know the location of your applet on the user's machine - either through guessing a cache path, tricking the user into downloading it before hand, or possibly using something like flash to create a file with a more deterministic name.

Other issues in this code:
 - <object> tags default codebase to the current directory, rather than domain-root, unlike <applet> tags. We need to emulate this quirk to avoid breakage like 736965
 - <param> tags appear to be passed as raw html to java, leading to potential disagreements between parsers on malformed HTML.
 - Some code checks for application/x-java-vm to trigger java quirks, but it is possible with <embed> to create a an applet with e.g. application/x-java-applet. The code that forces the MIME to x-java-vm only applies to <object> tags.
- nodePrinicpal()->GetURI() is returning NULL on object tags in one situation with <object> tags, causing security checks to be skipped.
- (probably non-issue) according to oracle docs, <applet> should follow HTML4 specs, which state "Values for [codebase] may only refer to subdirectories of the directory containing the current document". This is never enforced (except on file:/// URIs with the patch in 406541), and contradict's java's behavior of defaulting codebase to "/" for <applet>
Whiteboard: [sg:high]
Depends on: 745030
Keywords: sec-high
No longer depends on: CVE-2013-1717
Probably not reasonable to fix and uplift to Beta (16) at this point, but hopefully we can figure out a fix appropriate for 17.
John, we're a little over a week away from moving 17 to the Beta audience - any work done on this yet and if not can you give an idea of when you'll be working on it?
I've been waiting on landing more followups to this code until bug 783059 lands, as it's especially regression-prone (see original landing of bug 406541). I will try to have something ready before the uplift.
Status: NEW → ASSIGNED
Depends on: 783059
(In reply to John Schoenick [:johns] from comment #3)
> I've been waiting on landing more followups to this code until bug 783059
> lands, as it's especially regression-prone (see original landing of bug
> 406541). I will try to have something ready before the uplift.

Any updates here John? Is there something low risk we can do to resolve this issue in the FF17 timeframe?
I've emailed John about this.
Now that beta 5 has gone to build we're past taking speculative fixes. Wontfixing for 17.
Is this still a current sec-high issue?
John, what is the current status of this?  This is the oldest open DOM sec-high or sec-critical goal. :P
Flags: needinfo?(jschoenick)
Component: Security → Plug-ins
Flags: needinfo?(jschoenick)
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

So this got hairy fast, but I think should be decently-safe in that most of the changes reside within an |if (isJava)| block, so worst-case scenario is we break java...

The problem is multi-fold:
- Java uses "codebase" as a special value for where it is allowed to run, e.g. |file:///|, and assumes the browser will security-check it (despite not parsing it the way any browser would...)
- Java honors the last "codebase" value it sees, either on in tag attributes *or* in <param> children
- nsPluginInstanceOwner has a good deal of quirks relating to how it passes parameters and attributes to plugins
- nsObjectLoadingContent just looks at the attribute, not params, and without any of these quirks.
- Java doesn't parse URIs like the rest of the universe. E.g. no-codebase means ".", codebase="" means "/", and codebase="file:" means "file:///"

I opened Bug 853995 to move building the parameters we pass to the plugin to content (instead of having nsPluginInstanceOwner poke at the DOM), but that is non-trivial and wouldn't be suitable for branches. Instead, this patch basically copy-pastes a chunk of nsPluginInstanceOwner logic into nsObjectLoadingContent so they can arrive at the same codebase, with a patch for bug 853995 in progress to remove the code duplication from trunk.

Another issue is that we now parse the URI differently if we end up with Java, but can look at the URI's file extension in some cases and change plugin type. For example |<embed src="file.class">| will build file.class as a normal URI, before seeing .class and switching to java. I sort-of-elegantly fixed this by having UpdateObjectParameters re-call itself with a aJavaURI parameter at this point to redo the URI building steps with |isJava = true|.

Finally, now that we made a reasonable attempt to build the same mBaseURI that java would use, I added a java-only CheckLoadURIWithPrincipal security check against mBaseURI.

So risk-wise this patch looks scary, but is largely known-good code from nsPluginInstanceOwner, and fallout would likely only affect java plugins.
Attachment #731337 - Flags: review?(benjamin)
Comment on attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java

This causes nsPluginInstanceOwner to get the parsed-and-canonicalized codebase from nsObjectLoadingContent, and pass only that as a "codebase" value in the case of java plugins.

This guarantees that other, undiscovered quirks in how java determines its codebase would simply result in us overriding the codebase to our interpretation, instead of security-checking a different codebase than java decides to use.
Attachment #731338 - Flags: review?(benjamin)
Comment on attachment 731339 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

This would be folded against the canonicalize-codebase patch for branches only -- it moves the GetBaseURI() function to nsObjectLoadingContent.h and uses a nasty static-cast to get at it to avoid changing the nsIObjectLoadingContent IID on release branches.
Attachment #731339 - Flags: review?(benjamin)
This was previously tracked (and not fixed...), re-noming now that we have patches.
Too late for 20 at this point.
Inadvertently attached old version of this patch that doesn't compile...
Attachment #731339 - Attachment is obsolete: true
Attachment #731339 - Flags: review?(benjamin)
Attachment #731956 - Flags: review?(benjamin)
We'll track for the ESR once this is resolved. Since this is a sec-high and appears to carry some plugin risk, we'll want to land ASAP on branches and follow up with QA regression testing. Any guidance there would be very helpful.

If this isn't ready to land in time for our second or third beta, we should strongly consider sec-approving next cycle instead.
Keywords: qawanted, verifyme
QA Contact: mwobensmith
QA is ready to test when this gets landed. Basic concept is well-understood. Let me know if there are any other similar things to look for (param element vs object params, other types of paths, etc.).
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

I'd like a second review on these patches; tagging jst though he might redirect.
Attachment #731337 - Flags: superreview?(jst)
Attachment #731337 - Flags: review?(benjamin)
Attachment #731337 - Flags: review+
Comment on attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java

How do we test this?
Attachment #731338 - Flags: superreview?(jst)
Attachment #731338 - Flags: review?(benjamin)
Attachment #731338 - Flags: review+
Comment on attachment 731956 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

Kinda hacky, but I think we're safe here since web content can't spoof nsIObjectLoadingContent.
Attachment #731956 - Flags: review?(benjamin) → review+
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

- In nsObjectLoadingContent::UpdateObjectParameters():

+    mydomElement->GetElementsByTagNameNS(xhtml_ns, NS_LITERAL_STRING("param"),
+                                         getter_AddRefs(allParams));
+    if (allParams) {
+      uint32_t numAllParams;
+      allParams->GetLength(&numAllParams);
+      for (uint32_t i = 0; i < numAllParams; i++) {
+        nsCOMPtr<nsIDOMNode> pnode;
+        allParams->Item(i, getter_AddRefs(pnode));
+        nsCOMPtr<nsIDOMElement> domelement = do_QueryInterface(pnode);
+        if (domelement) {
+          nsAutoString name;
+          domelement->GetAttribute(NS_LITERAL_STRING("name"), name);
+          name.Trim(" \n\r\t\b", true, true, false);
+          if (name.EqualsIgnoreCase("codebase")) {
+            // Find the first plugin element parent
+            nsCOMPtr<nsIDOMNode> parent;
+            nsCOMPtr<nsIDOMHTMLObjectElement> domobject;
+            nsCOMPtr<nsIDOMHTMLAppletElement> domapplet;
+            pnode->GetParentNode(getter_AddRefs(parent));
+            while (!(domobject || domapplet) && parent) {
+              domobject = do_QueryInterface(parent);
+              domapplet = do_QueryInterface(parent);
+              nsCOMPtr<nsIDOMNode> temp;
+              parent->GetParentNode(getter_AddRefs(temp));
+              parent = temp;
+            }

You could do all this with far less reference counting and QI calls if you used NS_GetContentList() and the internal interfaces, but I'm not convinced it's worth rewriting this for that alone.

+    codebaseStr.AssignLiteral("/");
+    // XXX(johns): This doesn't cover the case of "https:" which java would
+    //             interpret as "https:///" but weinterpret as this document's

Insert the missing space in weinterpret.

sr=jst
Attachment #731337 - Flags: superreview?(jst) → superreview+
Comment on attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java

- In nsPluginInstanceOwner::EnsureCachedAttrParamArrays():

+  // (Bug XXXXXX FIXME) java has quirks in its codebase parsing, pass the
+  // absolute codebase URI as content sees it.

Replace XXXXXX with a bug number there?

+    mCachedAttrParamNames [nextAttrParamIndex] = ToNewUTF8String(NS_LITERAL_STRING("codebase"));
+    mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(NS_ConvertUTF8toUTF16(codebaseStr));

Wow, that's a pretty wonky string copy pattern we've got going here. Maybe file a followup bug to replace the above patterns with strdup() in this whole file?

sr=jst
Attachment #731338 - Flags: superreview?(jst) → superreview+
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #23)
> You could do all this with far less reference counting and QI calls if you
> used NS_GetContentList() and the internal interfaces, but I'm not convinced
> it's worth rewriting this for that alone.

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #24)
> Wow, that's a pretty wonky string copy pattern we've got going here. Maybe
> file a followup bug to replace the above patterns with strdup() in this
> whole file?

This is all just copy-pasted nsPluginInstanceOwner code -- I opened bug 853995 to clean this up and de-duplicate it.
(In reply to John Schoenick [:johns] from comment #25)
> (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #23)
> > You could do all this with far less reference counting and QI calls if you
> > used NS_GetContentList() and the internal interfaces, but I'm not convinced
> > it's worth rewriting this for that alone.
> 
> (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #24)
> > Wow, that's a pretty wonky string copy pattern we've got going here. Maybe
> > file a followup bug to replace the above patterns with strdup() in this
> > whole file?
> 
> This is all just copy-pasted nsPluginInstanceOwner code -- I opened bug
> 853995 to clean this up and de-duplicate it.

Yup, didn't mean to suggest that this was your doing, it's all over this file from way long ago (and you did the right thing in following the existing coding patterns here)! Thanks for filing the followup!
Dropping qawanted but leaving verifyme. Matt is assigned to verify this once it lands.
Keywords: qawanted
Fix possible off-by-one, carrying over r
Attachment #731338 - Attachment is obsolete: true
Attachment #740385 - Flags: superreview+
Attachment #740385 - Flags: review+
Given https://bugzilla.mozilla.org/show_bug.cgi?id=738396#c18 and considering we are just two beta's away for Fx21 wontfixng this for fx21 . This will be a better fit for next cycle .
Hello all - any movement here?
Flags: needinfo?(jschoenick)
Attached patch Add nptest java plugin (obsolete) — Splinter Review
Flags: needinfo?(jschoenick)
Attached patch Test (obsolete) — Splinter Review
Comment on attachment 755508 [details] [diff] [review]
Add nptest java plugin

Adds another target for nptest that claims x-java-test, and adds a getJavaCodebase function, so we can test that we're passing the intended codebase to java tags.
Attachment #755508 - Flags: review?(benjamin)
Comment on attachment 755510 [details] [diff] [review]
Test

Uses the new java test plugin target to ensure we see the expected codebase in various situations.
Attachment #755510 - Flags: review?(benjamin)
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Pretty clearly reveals we are mis-handling java codebase, if the linked bug is also a restricted bug, this would quickly give someone the information they need to figure out the issue.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The code is pretty clear that we didn't audit java's codebase previously.

Which older supported branches are affected by this flaw?
All current branches. Irrelevant to b2g as we don't support plugins there.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes. Some of the riskier changes are replaced with simplier workarounds for branches only (the foldme patches)

How likely is this patch to cause regressions; how much testing does it need?
Low-to-moderate chance of introducing regressions, which would likely only affect certain configurations of java embedding.
Attachment #731337 - Flags: sec-approval?
Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

[Security approval request comment]
(See above)
Attachment #740385 - Flags: sec-approval?
Comment on attachment 731956 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

[Security approval request comment]
See above. This just avoids changing the nsIObjectLoadingContent IID on branches.
Attachment #731956 - Flags: sec-approval?
(In reply to John Schoenick [:johns] from comment #35)
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> The code is pretty clear that we didn't audit java's codebase previously.
>
> How likely is this patch to cause regressions; how much testing does it need?
> Low-to-moderate chance of introducing regressions, which would likely only
> affect certain configurations of java embedding.

Give this, I think we should be cautious and fix for the first time in 23/24/25 and the corresponding ESR, to get a full 6 weeks of testing before release.
Whiteboard: [sg:high] → [sg:high][land once m-c is FF25]
The problem is

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Pretty clearly reveals we are mis-handling java codebase, if the linked bug
> is also a restricted bug, this would quickly give someone the information
> they need to figure out the issue.
> 
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> The code is pretty clear that we didn't audit java's codebase previously.

This means that once we check in, we're going to be pretty exposed for the whole six week cycle.
Given this bug's potential impact (this is less severe than remote code execution, and isn't necessarily easy to exploit), I think favoring extra bake time over reduced trunk exposure seems reasonable.
There is also bug 406541, which is a much less severe bug that depends on the changes here to fix. We could open a public version of that bug, then land these changes folded with 406541 as a "cleanup java codebase handling" patch. Without a locked bug to bring attention to it and the valid-premise for cleaning up this code, it's less obvious that there's a more severe issue to be found.
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

Approving for trunk AFTER trunk becomes Firefox 25 on 6/24 or 25.
Attachment #731337 - Flags: sec-approval? → sec-approval+
Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

Approving for trunk AFTER trunk becomes Firefox 25 on 6/24 or 25.
Attachment #740385 - Flags: sec-approval? → sec-approval+
Whiteboard: [sg:high][land once m-c is FF25] → [sg:high][checkin after 6/25]
Attachment #731956 - Flags: sec-approval?
Attachment #755508 - Flags: review?(benjamin) → review+
Comment on attachment 755510 [details] [diff] [review]
Test

The printf in nptest.cpp seems like a testing leftover? r=me with that gone
Attachment #755510 - Flags: review?(benjamin) → review+
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

https://hg.mozilla.org/integration/mozilla-inbound/rev/de553526d1f8
Attachment #731337 - Flags: checkin+
Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d38d2a6e4c1
Attachment #740385 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/de553526d1f8
https://hg.mozilla.org/mozilla-central/rev/5d38d2a6e4c1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 888834
Attached patch Test. r=bsmedberg (obsolete) — Splinter Review
Folds in a few more cases for the regression in bug 888834, carrying over r+
Attachment #755510 - Attachment is obsolete: true
Attachment #769872 - Flags: review+
Flags: in-testsuite?
Whiteboard: [sg:high][checkin after 6/25] → [sg:high]
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Technically a java bug: It assumes the browser will security-check its plugin-specific parameters, but we do not.

User impact if declined:
sec-high. Potential for untrusted java applets to get read-only access to the user's filesystem if used in conjunction with some method to download a file to a guessable path.

Testing completed (on m-c, etc.):
Has test case, on m-c. One minor regression found in bug 888834, which is now fixed and would be folded into the uplift patch.

Risk to taking this patch (and alternatives if risky):
In relation to java, there is a moderate regression risk. Java does not construct its "codebase" attribute in a very sane fashion, mis-constructing URLs and assuming |codebase=""| means |codebase="/"| among other things. Because we now force agreement on the codebase parameter, any quirks in java we did not emulate properly would break some java applets (e.g. bug 888834). These would be trivial fixes, if found.

Outside of embedded java, the risk is low, as all new code only applies to Java.

String or IDL/UUID changes made by this patch:
None
Attachment #731337 - Flags: approval-mozilla-esr17?
Attachment #731337 - Flags: approval-mozilla-beta?
Attachment #731337 - Flags: approval-mozilla-aurora?
Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

(see comment 49)
Attachment #740385 - Flags: approval-mozilla-esr17?
Attachment #740385 - Flags: approval-mozilla-beta?
Attachment #740385 - Flags: approval-mozilla-aurora?
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

after 6/25 so let's uplift.
Attachment #731337 - Flags: approval-mozilla-esr17?
Attachment #731337 - Flags: approval-mozilla-esr17+
Attachment #731337 - Flags: approval-mozilla-beta?
Attachment #731337 - Flags: approval-mozilla-beta+
Attachment #731337 - Flags: approval-mozilla-aurora?
Attachment #731337 - Flags: approval-mozilla-aurora+
Attachment #740385 - Flags: approval-mozilla-esr17?
Attachment #740385 - Flags: approval-mozilla-esr17+
Attachment #740385 - Flags: approval-mozilla-beta?
Attachment #740385 - Flags: approval-mozilla-beta+
Attachment #740385 - Flags: approval-mozilla-aurora?
Attachment #740385 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/819cefc60ade
https://hg.mozilla.org/releases/mozilla-aurora/rev/20a1cd5be4fb

There are some conflicts in nsObjectLoadingContent.cpp on esr17 that will need resolving. Also, this requires binary approval to land on beta due to the changes to nsIObjectLoadingContent.idl (comment 49 said no IDL changes?!?!).
Flags: needinfo?(jschoenick)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #52)
> Also, this requires binary approval to land on beta due to
> the changes to nsIObjectLoadingContent.idl (comment 49 said no IDL
> changes?!?!).

This is why the |Avoid changing IID on branches| patch exists. I should've clarified in the whiteboard, apologies.

https://hg.mozilla.org/releases/mozilla-beta/rev/24df0a207416
https://hg.mozilla.org/releases/mozilla-beta/rev/10544c91e1d6
Flags: needinfo?(jschoenick)
QA has completed an extensive pass on this bug and signs off.

Confirmed original path loading exploit on OpenJDK6, Linux, FF22.
Verified fixed in FF25 m-c, 2013-07-08.
Verified fixed in FF24, 2013-07-10.
Verified fixed in FF23, 2013-07-10.
Verified fixed in FF17esr, 2013-07-10.

Verified that:
- Bad paths prevent JRE from loading applet at all, or
- Bad paths cause applet not to display, with error from JRE (security error or class not found).
- Normal use cases for applet loading - via object, embed and applet tags - work correctly.
- Other test cases for applet loading - nested tags, redirects, site vs root relative URLs, edge cases - work correctly.

JDKs used:
OpenJDK6u45/Ubuntu 13.0.4
OpenJDK7u21/Ubuntu 13.0.4
Oracle JDK 7u21/Ubuntu 13.0.4
Oracle JDK 7u25/Ubuntu 13.0.4
Oracle JDK 7u21/Mac OSX 10.8
Oracle JDK 7u25/Mac OSX 10.8
Whiteboard: [sg:high] → [sg:high][adv-main23+][adv-esr1708+]
Duplicate of this bug: 964055
The blog post linked in bug 964055 doesn't talk about Java plugins at all.
Un-bitrotted, rebased on java test plugin which was split out and added in bug 971279
Attachment #755508 - Attachment is obsolete: true
Attachment #769872 - Attachment is obsolete: true
Attachment #8379263 - Flags: review+
(In reply to Wes Kocher (:KWierso) from comment #61)
> I backed this (and everything else from that push to inbound) out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/128cbf1edc40 due to
> various Java/plugin related failures they caused:
> Crashtest failure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35003251&tree=Mozilla-
> Inbound
> XPCShell failure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35003355&tree=Mozilla-
> Inbound

Attempt 2 to push test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a4abd04dce

Extensive try run since this push burned the tree before:
https://tbpl.mozilla.org/?tree=Try&rev=b656bdd77ce1
Flags: needinfo?(jschoenick)
This was fixed in 25, the additional landing here only applies to the mochitest
Flags: in-testsuite? → in-testsuite+
Group: core-security
You need to log in before you can comment on or make changes to this bug.