bcit-ci / CodeIgniter

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
18.27k stars 7.6k forks source link

Unescaped HTML (cross-site scripting) from database errors #5848

Closed joshkel closed 4 years ago

joshkel commented 5 years ago

We noticed that, when a bad SQL query contained strings with HTML tags, the database error page tried to render that HTML. For example:

Error Number: 42000/207

[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Invalid column name 'bad_field'.

SELECT count(*) as cnt FROM sample_view WHERE bad_field = 'hello'

Filename: models/Sample_model.php

Line Number: 323

when the actual query was

SELECT count(*) as cnt FROM sample_view WHERE bad_field = '<b>hello</b>'

I believe that the fix is for DB_driver::query's call to display_error to call html_escape on the database code and message.

The other option would be for CI_Exceptions::show_error to call html_escape on each part of its $message parameter when showing an HTML template. However, some callers may rely on the ability to pass HTML tags to it, so I'm guessing this would be less desirable.

I'm happy to open a PR with whichever solution makes sense.

Thank you.

Elardzhi commented 5 years ago

Displaying errors must be on only for you (on DEV server).

joshkel commented 5 years ago

@Elardzhi Thanks; I know that displaying errors should be turned off in production. So, in terms of security risk, it's low. But it is a bug.

narfbg commented 5 years ago

Should rather handle this in the appropriate error template - there's separate ones for HTML and CLI. You can just use htmlspecialchars() (same params as in the Profiler library); if there's a security concern here at all, it's that one would have both display_errors and $config['db_debug'] enabled in production, not XSS.

narfbg commented 4 years ago

Closing due to lack of feedback.

joshkel commented 4 years ago

Hi. Sorry for not replying sooner.

So, if this should be handled in the appropriate error template, then would you be interested in a PR to add htmlspecialchars() to the default html/error_db.php template that ships with CodeIgniter?

I do understand that it's a misconfiguration to enable display_errors in production, and so I apologize if I muddied the waters or miscommunicated by referencing XSS. But I also believe that it's a bug for CodeIgniter's framework code or standard application code to take arbitrary text data and display it unescaped as HTML.

narfbg commented 4 years ago

I mean ... I disagree about classifying it as a bug, but I get where you're coming from and sure - a PR that only modifies the HTML template would be just fine.