CVE Fix - XSS in PathLength attribute in CA agent web page #446

SilleBille · 2020-06-15T22:54:22Z

Clean out Path Length field in CA's Agent Web page

Signed-off-by: Dinesh Prasanth M K <dmoluguw@redhat.com>

jmagne

This looks like it gets the job done. I would ask that maybe we could check if those math functions being used are supported fully in any of our supported browsers..

cipherboy

This validation should occur on the server side. Validation on the client side is fine for a message to the user, but ultimately it should be occurring on the server side.

Edit: Additionally, you can use the number typed input element instead of this client-side validation. However, you still need server-side validation.

cipherboy · 2020-06-16T18:04:25Z

base/ca/shared/webapps/ca/agent/ca/ProfileReview.template

+function validateForm(form) {
+  pathLength = form.basicConstraintsPathLen;
+
+  if(pathLength != null && !Number.isInteger(parseInt(pathLength.value))) {


Number.isInteger wrapping parseInt is excessive and redundant. :)

This validation should occur on the server side. Validation on the client side is fine for a message to the user, but ultimately it should be occurring on the server side.

There is server side validation already included. The web UI throws this message when you try to enter anything funky

The Certificate System has encountered an unrecoverable error. Error Message: java.lang.NumberFormatException: For input string: "" Please contact your local administrator for assistance

The CVE is to prevent execution of malicious JS on the client browser. So, my patch is trying to clean out before sending the values to the server.

Also, note that I couldn't test this with anyother value other than -1. I filed a bug: https://pagure.io/dogtagpki/issue/3179

Edit: Additionally, you can use the number typed input element instead of this client-side validation. However, you still need server-side validation.

I can try that

Then it isn't clear to me where exactly the vulnerability occurs? Is it because the error message is displayed incorrectly (getting interpreted as HTML)? If so, was it fixed by #434? If not, we should fix the site that displays the error message.

Otherwise, I really don't see how this is worthy of a CVE: user types at computer and attacks themselves. Closed->NOTABUG. You don't need RHCS for that. You can do that with bash or literally anything else. Or directly on the web console.

Is this XSS stored? Does it get saved for an agent to stumble upon later? (i.e., if you make this page, and then access it as another user / with another browser and same user -- do you hit the same XSS again?) -- Then you need to sanitize the incorrect value before saving it in the database and also sanitize where it gets displayed to the agent. But this isn't the proper fix for it if so.

Yeah, good point. Someone could be using the pki cli command for some of this stuff.

I was referring to the comment above about server side checking.

Also perhaps he is using the isInteger to handle the case if parseInt returns the Nan value, which I guess is not a number.

Ah I figured this out. The server endpoint doesn't do any CSRF protection and also responds to GET requests. This truly makes it a reflected XSS, which is bad. The fix is three part:

Sanitize the output error message,

Enable CSRF protection,

Optionally handle only POST requests.

- The input type is set to number when "integer" is encountered - The server error message is html escaped, before it gets displayed in client browser Resolves: BZ#1710171 Signed-off-by: Dinesh Prasanth M K <dmoluguw@redhat.com>

SilleBille · 2020-06-18T00:58:34Z

@cipherboy Seems like IPA doesn't like my patch. They are submitting cert approval request via GET. update: After talking to Fraser, I have filed a IPA ticket: https://pagure.io/freeipa/issue/8373

I am not familiar with enabling CSRF protection. I tried to google and found only ways to enable it in a Spring framework. IIUC, we don't use spring. Do you have any docs which I can refer to?

OTOH, nonce value is attached to the request. Can we not use that as our unique token to get our request processed?

frasertweedale · 2020-06-18T05:54:04Z

The issue was in Certmonger. There is a pull request: https://pagure.io/certmonger/pull-request/157. We need to wait for it to be merged and for new release of certmonger.

SilleBille · 2020-06-18T16:22:27Z

Thanks, Fraser! I have reverted the commit for now and will push it as part of separate PR.

@cipherboy @jmagne as discussed on IRC, out of the 3 part fix that Alex mentioned, only the first is addressed in this PR. The second is already handled via the nonce value attached to the request; the tomcat's CSRF filter will be enabled later via different PR. The third will be pushed via separate PR, once the new version of certmonger gets released with the changes introduced by Fraser

cipherboy

👍 Looks good to me, thanks Dinesh for updating!!

SilleBille · 2020-06-18T17:37:57Z

Merging based on the ACK and green light from CI! Thank you for the reviews and suggestions

jmagne

Iknow you merged, but looks good..

SilleBille requested review from cipherboy and jmagne June 15, 2020 22:54

SilleBille self-assigned this Jun 15, 2020

SilleBille added the Bug Bug fixes label Jun 15, 2020

jmagne reviewed Jun 16, 2020

View reviewed changes

cipherboy suggested changes Jun 16, 2020

View reviewed changes

cipherboy reviewed Jun 16, 2020

View reviewed changes

SilleBille force-pushed the CVE-2019-10221 branch from 19e1ec9 to 2faf105 Compare June 16, 2020 21:39

Fix XSS in PathLength attribute in CA agent web page

6e5572f

- The input type is set to number when "integer" is encountered - The server error message is html escaped, before it gets displayed in client browser Resolves: BZ#1710171 Signed-off-by: Dinesh Prasanth M K <dmoluguw@redhat.com>

SilleBille force-pushed the CVE-2019-10221 branch from 2faf105 to 6e5572f Compare June 17, 2020 22:36

SilleBille force-pushed the CVE-2019-10221 branch from be18898 to 6e5572f Compare June 18, 2020 14:31

cipherboy approved these changes Jun 18, 2020

View reviewed changes

SilleBille merged commit b235c0f into dogtagpki:master Jun 18, 2020

SilleBille deleted the CVE-2019-10221 branch June 18, 2020 18:03

jmagne reviewed Jun 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CVE Fix - XSS in PathLength attribute in CA agent web page #446

CVE Fix - XSS in PathLength attribute in CA agent web page #446

SilleBille commented Jun 15, 2020

jmagne left a comment

cipherboy left a comment •

edited

cipherboy Jun 16, 2020

SilleBille Jun 16, 2020

cipherboy Jun 16, 2020 •

edited

jmagne Jun 16, 2020

jmagne Jun 16, 2020

jmagne Jun 16, 2020

cipherboy Jun 17, 2020

SilleBille commented Jun 18, 2020 •

edited

frasertweedale commented Jun 18, 2020

SilleBille commented Jun 18, 2020 •

edited

cipherboy left a comment

SilleBille commented Jun 18, 2020

jmagne left a comment

CVE Fix - XSS in PathLength attribute in CA agent web page #446

CVE Fix - XSS in PathLength attribute in CA agent web page #446

Conversation

SilleBille commented Jun 15, 2020

jmagne left a comment

Choose a reason for hiding this comment

cipherboy left a comment • edited

Choose a reason for hiding this comment

cipherboy Jun 16, 2020

Choose a reason for hiding this comment

SilleBille Jun 16, 2020

Choose a reason for hiding this comment

cipherboy Jun 16, 2020 • edited

Choose a reason for hiding this comment

jmagne Jun 16, 2020

Choose a reason for hiding this comment

jmagne Jun 16, 2020

Choose a reason for hiding this comment

jmagne Jun 16, 2020

Choose a reason for hiding this comment

cipherboy Jun 17, 2020

Choose a reason for hiding this comment

SilleBille commented Jun 18, 2020 • edited

frasertweedale commented Jun 18, 2020

SilleBille commented Jun 18, 2020 • edited

cipherboy left a comment

Choose a reason for hiding this comment

SilleBille commented Jun 18, 2020

jmagne left a comment

Choose a reason for hiding this comment

cipherboy left a comment •

edited

cipherboy Jun 16, 2020 •

edited

SilleBille commented Jun 18, 2020 •

edited

SilleBille commented Jun 18, 2020 •

edited