Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#374 SQL Injection Vulnerability

Development_Release
open-fixed
nobody
security (1)
1
2013-11-23
2013-11-22
Patrick Smith
No

I found a SQL Injection vulnerability due to an unescaped usage of the language_choice session variable. This session variable is defined from the language selection field on the home page but no validation is completed to ensure that it contains a safe value.

It seems a short-term fix is to escape the usage of the language_choice parameter in any subsequent queries and the longer-term fix is to validate the user input and set the value to a default if invalid input is received.

Discussion

  • Kevin Yeh
    Kevin Yeh
    2013-11-22

    I see at least two places where this issue should be addressed to tighten things

    https://github.com/openemr/openemr/blob/master/library/auth.inc#L44
    Should verify that we have numeric input here.

    and here:

    https://github.com/openemr/openemr/blob/master/library/translation.inc.php#L35
    Escape the input

    Did you construct an exploit using $POST['language_choice'] that does in fact create a SQL error though?

    I'm trying to figure out the urgency with which we need to issue a patch. translation.inc.php gets included before "auth.inc.php" in interface/globals.php so it might be possible to run malicious SQL without valid OpenEMR credentials in this case.

     
  • Patrick Smith
    Patrick Smith
    2013-11-22

    Yes, I was able to construct an exploit using $POST['languageChoice'] that allows for data leakage.

    I believe that the issue can be fixed in translation.inc.php by updating to...

    $sql="SELECT * FROM lang_definitions JOIN lang_constants ON " .
    "lang_definitions.cons_id = lang_constants.cons_id WHERE " .
    "lang_id='?' AND constant_name = '" .
    add_escape_custom($constant) . "' LIMIT 1";
    $res = sqlStatementNoLog($sql, $lang_id);

    or something similar.

    The above code should just throw an SQL exception. The validation part of auth.inc will make sure the error no longer occurs at all.

    I wasn't able to leak data without logging in with valid credentials. I believe the only location that the $_SESSION['language_choice'] is set is in auth.inc so I don't believe it is possible to leak data without valid credentials (at least with this vulnerability).

    I have added the above fix but my lack of git skillz has prevented me from pushing it back to the master repository. I am not sure if there is a process for you guys to allow for new committers either as I haven't committed any fixes to this project before.

    At any rate, I hope all of this helps.

     
  • Kevin Yeh
    Kevin Yeh
    2013-11-22

    Patrick,
    Thanks. This is EXTREMELY helpful.

    Although you couldn't leak data without a valid login, since you were able to generate an error, that implies that modifying/inserting data is possible. Someone clever could create login credentials if the "privileged query/user MySQL" mechanism hasn't been setup per these instructions for 4.1.2. Simpler would be to create a destructive query.

    https://github.com/openemr/openemr/blob/master/Documentation/privileged_db/priv_db_HOWTO.txt

    Which is something I suspect hasn't been done on most servers.

    Older versions are vulnerable.

    Properly escaping the query in translation.inc.php as you indicated is EXACTLY what needs to happen at a minimum.

    The proper "process" is for you to make the commit in your own repository. If you have a link to share, one of the integration developers (myself, Brady, Tony or Rod) will then bring it in to the code base. One of us will do either a pull or a cherry-pick from your commit.

     
  • Patrick Smith
    Patrick Smith
    2013-11-22

    Kevin, thanks for the primer on the process.

    However, I will be out of pocket for a few days and will be unable to get this set up anytime soon so it will probably be a lot faster / easier for you to just make the change. What I have described so far is all that I had updated at this point anyway.

     
  • Kevin Yeh
    Kevin Yeh
    2013-11-22

    Patrick,
    In testing this, the fix also needed to switch add_escape_custom($constant) to use binding as combining the two techniques can cause problems if there are special characters (like a ?) in a $constant, and there is another potential vulnerability

    I'm actively working on this right now. Your "contribution" isn't going to be directly reflected in the commit history in github by having me push into the official repository. Not a "big deal," but you definitely deserve some real credit here.

    When you have a less time critical commit in the future, we can work on the proper process.

    Thanks again.

     
  • Brady Miller
    Brady Miller
    2013-11-23

    Hi,

    Thanks for the vulnerability report and quick fix.

    Although it may not clearly be a pre-authentication vulnerability, I think it does get used in some scripts that bypass authentication.

    To expedite things, I committed Kevin and Patrick's fix into development branch (ie. 4.1.3) and just released the fix in a 4.1.2 patch:
    http://www.open-emr.org/wiki/index.php/OpenEMR_Patches

    -brady
    OpenEMR

     
  • Brady Miller
    Brady Miller
    2013-11-23

    • status: open --> closed-fixed
     
  • Brady Miller
    Brady Miller
    2013-11-23

    • status: closed-fixed --> open-fixed
     
  • Brady Miller
    Brady Miller
    2013-11-23

    Was gonna close this but realized it was also suggested to do some validation of user input. So will leave it open. Feel free to close it if don't think that is needed.