#44 guard against xss vunlerability in patient name (again)

closed-fixed
nobody
None
5
2010-06-29
2010-06-02
Andrew Moore
No

This is the second attempt to patch against one more of the billion XSS
vulnerabilities. They're everywhere!

This was reported by David Shaw at Redspin, Inc. and documented at:
https://sourceforge.net/projects/openemr/forums/forum/202506/topic/3530656

I created a patient with name of "<script>alert("broken");</script>"
and then fixed all of the instances where I got an alert by browsing
around the application. There may be more.

To reduce these in the future, please wrap any user-submitted data in
htmlspecialchars() before displaying it.

Discussion

  • Brady Miller
    Brady Miller
    2010-06-04

    line 47 of patch still has the evil htmlentities function :)
    (you have privileges to delete and/or add multiple files to any tracker item)

     
  • Andrew Moore
    Andrew Moore
    2010-06-04

    Thanks for your keen eye, Brady.
    I've resubmitted the patch.
    -Andy

     
  • Andrew Moore
    Andrew Moore
    2010-06-04

    Following up on a comment from Brady in another thread:

    - Yes, this patch was made against the development HEAD. It still applied cleanly when I last tried
    - I believe this is ready to commit whenever you think it is. I don't know anything else that needs to be done to it.

    Thanks,
    -Andy

     
  • Brady Miller
    Brady Miller
    2010-06-04

    hey,
    I'll need to look at this in more detail. The changes to options.inc.php and patient.inc may be to "deep" within the code. Goal is to only capture stuff that goes to screen, and avoid doing this to stuff that may get "processed" and avoid doing it twice on same string (not sure what would happen if done twice?).
    -brady

     
  • I noticed you are using htmlspecialchars() inside a script element. This will likely work for many browsers, even though it shouldn't. In the HTML 4 DTD the script element was given a content type of CDATA, which means that entities (e.g. &amp;) and markup aren't recognized except for the matching end tag. Unfortunately we can't completely count on that, since XHTML, being XML rather than SGML, doesn't support this method of preventing entities and markup from being interpreted. So, depending on how conforming the browser is and what it thinks your document is, your entities may or may not be recognized.

    Better is to make sure that all your script blocks start with "//<![CDATA[" on a line by itself and end with //]]> on a line by itself, and do *not* use htmlspecialchars() or other ways of using entities. If the browser handles the contents of the script element as CDATA, then these lines will simply be passed to the JS intrepreter and dropped as comments, you *will* need to make sure a script doesn't contain "</", even within a character string or comment, but you can't use entities to do it. If the browser handles the document as XML/XHTML, the "<![CDATA[" will turn off entity and markup recognition until the "]]>". In addition to what is in the CDATA Section, the JS interpreter will get a leading "//". You *will* need to make sure a script doesn't contain "]]>", even within a character string or comment, but you can't use entities to do it.

    The only time this would break would be with old of buggy browsers that treat the content of a script tag as PCDATA (and therefore, recognize entities and markup there) AND do not recognize the "<![CDATA[", which is a part of both SGML and XML (but was not generally mentioned in the documentation for HTML versions before 4.)

    In short, instead of using htmlspecialchars() we need a function that does something special with "</" and "]]>" and to fix up our JS to have the right leader and trailer.

     
  • I'm not saying you have to fix up all the PHP-generated JS right now. But, since you are modifying these blocks, we should try and do it right at least in them. It would be nice to have a jsString() function that did at least the following:
    "'" -> "'\''"
    "</" -> "('<' + '/')"
    "]]>" -> "(']]' + '>')"
    I'm not sure what other transformations you'd need to do to a PHP string to make it be a PHP string containing a JS expression that evaluated to the same series of characters.

     
  • Brady Miller
    Brady Miller
    2010-06-04

    My vote is to just focus on the directed bug fix for now. Stephen is bringing up more of a global issue (that i need to read up on...). It sounds like a valid global issue that will require some more global functions. Probably shouldn't slow down this bug fix.
    -brady

     
  • Andrew Moore
    Andrew Moore
    2010-06-09

    Stephen - Thanks for the info on the CDATA blocks. I thought they weren't necessary anymore, but perhaps they are to support older browsers. I have to defer to your knowledge.
    Would you be OK with us committing a fix like I have submitted that fixes this security problem in most browsers, and then later working to make our JS more palatable to more browsers?

    Brady - You mentioned that you wanted to look at this in more detail since the options.inc.php and patient.inc changes may affect more than anticipated. Is that something you're still interested in doing, or are you comfortable with this patch?

    Thanks for your help, guys!

    -Andy

     
  • Brady Miller
    Brady Miller
    2010-06-12

    Andy,
    sorry for the delay. still gotta look through the options.inc.php stuff. should get through it by tomorrow.
    -brady

     
  • Brady Miller
    Brady Miller
    2010-06-14

    • assigned_to: nobody --> bradymiller
     
  • Brady Miller
    Brady Miller
    2010-06-15

    The options.inc.php code I agree with, but as you know this is only the tip of the iceberg. The cool thing is that when we use htmlspecialchars() properly throughout all the functions in options.inc.php (in future, of course, this is not scope of this patch) we will have killed a large number of the xss vulnerabilities in one fell swoop.

    However, I think that the patient.inc getPatientName() change is too deep within the code. We should really just be using the htmlspecialchars() function on items that we know are going to screen output. For example, code may call the getPatientName function and then insert it into the database as an identifier (ie. never output it to the screen). So, instead need to htmlspecialchars() wherever it is outputted to the screen.

    I put your patch on my github repository here:
    http://github.com/bradymiller/openemr/tree/patch-3010645_1

    -brady

     
  • Andrew Moore
    Andrew Moore
    2010-06-17

    Thanks, Brady.
    I moved the call to htmlspecialchars() outside of getPatientName and put it around the few calls to that function that I could find. I also found a few more places that needed escaping, mostly when getPatientData was called.

     
  • Brady Miller
    Brady Miller
    2010-06-17

    hey,
    code looks good. If this tests well for you, then it's commitable. No need for me to test it, but it will be easier for me to commit it (so I can record each version number of file to then bring over to version 3.2). Just let me know that it tests fine for you, and I'll commit it.
    thanks,
    -brady

     
  • Brady Miller
    Brady Miller
    2010-06-28

    • assigned_to: bradymiller --> nobody
    • status: open --> open-fixed
     
  • Andrew Moore
    Andrew Moore
    2010-06-29

    • status: open-fixed --> closed-fixed