3 ms·

I feel like this article is conflating things... it is absolutely possible to do SQL string concatenation safely. I've done it many times to work around aggres

by Pxtl 7y ago

I feel like this article is conflating things... it is absolutely possible to do SQL string concatenation safely. I've done it many times to work around aggressively-bad SQL APIs. Assuming SQL concatenation is automatically bad is the kind of thinking that makes me roll my eyes at security researchers.

bitexploder 7y ago
For every person that gets SQL strong concatenation right 20 get it wrong. And there are simple and safe ways to write arbitrary SQL queries without using concatenation. Parmeterized queries are available everywhere. There is a reason we tell everyone not to do it and that reason is that it’s dangerous and almost everyone screws it up. I have found your case to be the exception in 13 years as an infosec researcher and consultant.
shakna 7y ago
Building SQL queries from strings is possible safely, in the same way that C's memory model is safe. It can be done... But not by most people. Are history proves it, time [0] after time [1] and time [2] after time [3]. It is simply not worth it, if you can make use of parameterized queries. [0] https://web.archive.org/web/20161024090111/https://ico.org.uk/about-the-ico/news-and-events/news-and-blogs/2016/10/talktalk-gets-record-400-000-fine-for-failing-to-prevent-october-2015-attack/ https://web.archive.org/web/20161024090111/https://ico.org.u... [1] https://www.nytimes.com/2014/08/06/technology/russian-gang-said-to-amass-more-than-a-billion-stolen-internet-credentials.html?_r=0 https://www.nytimes.com/2014/08/06/technology/russian-gang-s... [2] https://www.baltimoresun.com/education/bs-xpm-2014-03-07-bs-md-hopkins-servers-hacked-20140306-story.html https://www.baltimoresun.com/education/bs-xpm-2014-03-07-bs-... [3] https://web.archive.org/web/20150219165019/http://www.batblue.com/united-nations-internet-governance-forum-breached/ https://web.archive.org/web/20150219165019/http://www.batblu...
- jackewiehose 7y ago
  > Building SQL queries from strings is possible safely, in the same way that C's memory model is safe. But still you wouldn't say that every software written in C is automatically insecure. You have to look into the actual case. Just grepping for SQL-string-concatination or uses of sprintf doesn't say much.
  - Grollicus 7y ago
    The difference is that there are sometimes no real alternatives to doing memory management by hand - which means there are prople that are really really really good at this. With sql that's different, every sane sql client library supports some form of prepared statements out of the box and they are really simple to use. There is no reason whatsoever to do this by hand, except for people that don't know better.
    - Pxtl 7y ago
      > every sane sql client library supports some form of prepared statements out of the box this is true > they are really simple to use this is not. The amount of boilerplate I've written to properly parameterize a common pattern like SELECT * FROM FOO WHERE FOO.SomeKey IN (/* user-provided list of chosen values goes here */) is grotesque.
      - pdkl95 7y ago
        I'm not sure I would classify a sql client library that doesn't support arrays as "sane". This may have been annoying in the past, I would expect modern libraries to handle basic features like 'IN" automagically. list = ['bar', 'baz', "Robert'); DROP TABLE Students;--"] Foo.where(name: list) # Foo Load (0.3ms) # SELECT "foo".* FROM "foo" # WHERE "foo"."name" IN (?, ?, ?) # [["name", "bar"], # ["name", "baz"], # ["name", "Robert'); DROP TABLE Students;--"]]
    - ufmace 7y ago
      I disagree with the part that there is always an alternative to building strings for SQL statements. Yes, all client libraries support prepared statements. But none of them support placing prepared statement fields in any part of the SQL statement. Prepared statements work fine as long as you just have simple SELECT statements with a few WHERE values as parameters. They completely fail if you need to do any advanced SQL with anything dynamic. Like optionally add more sophisticated calculated properties to the SELECT fields, conditionally JOIN in extra tables, use any of the DB engine's more advanced XML or JSON parsing features in a conditional way, support choosing <, > or =, etc.
EvanAnderson 7y ago
Setting aside the security ramifications of getting SQL string concatenation wrong, your RDBMS query planner may be able to cache a paramaterized query plan, whereas a query built by string concatenation may be seen as a new query to be compiled and optimized.
- Pxtl 7y ago
  Maybe it's just Microsoft's SQL server, but I've come to become extremely suspicious of this. I've run into far too many cases where, despite our DBA playing with cache-clearing strategy and statistics and the like, we found that queries would run fine in the unparameterized query-console and would run with pathological performance in the actual application. Every time it's "hey mr senior developer, I'm trying to reproduce that slow query but I can't, even though I'm doing the same thing as the application" and I'm like "you must be doing something wrong" and sure enough: no, it's the SQL server doing something wrong. And every time we found out "oh, running it un-parameterized makes it fast". So then I have to talk down the juniors who are like "well fine, I can fix it by running it unparameterized, let's just do that!". So then we have to figure out how to trick it to generating a non-idiotic query plan without abandoning the security that parameterization provides. ... basically I'm deep into "the emperor has no clothes" on MS SQL server in general.
  - bcrosby95 7y ago
    This is like a window into my current problems. We've never used SQL Server, but a vendor for a client requires it. We are constantly running into this problem. Right now we have a query that takes 300ms when we back out the parameters, or ~12 minutes when we parameterize it. It is extremely frustrating. Never ran into problems like this with Postgres or MySQL.
maxlybbert 7y ago
I agree. SQL string concatenation that combines programmer-controlled strings with potentially-user-controlled numbers, for instance, is immune from SQL injection. And it’s a connection to a local SQLite database for session data and configuration info, so SQL injection wouldn’t really be able to do much (later the report says the database has history, logs, and “probably also sensitive data such as passwords” but doesn’t say what evidence there is that there’s sensitive data in the database). The sprintf example acknowledges that it’s not clear if the strings involved are user-controlled. It’s definitely possible that the programmer knows all the strings that may be involved and can prove that buffer overflow is impossible. There’s the complaint that Zoom crash reporting reads registry values that the application has permission to read. This is only problematic because it sends that information to Zoom’s crash reporting service; i.e., somebody other than the user. But programs if the program can’t write sensitive data to the registry, then there’s nothing sensitive to disclose.
- xmodem 7y ago
  Take a look at this defcon talk: https://www.youtube.com/watch?v=JokZUjwGj4M https://www.youtube.com/watch?v=JokZUjwGj4M If message history goes into that database and is not sanitised properly there's a good chance this gives other conference participants RCE.
  - maxlybbert 7y ago
    I work at a bank, and we have strict rules about what data can be logged (and what data can never be logged), to avoid accidental information disclosure if somebody ever gets our logs (which, I assume, would be about as hard to do as getting somebody’s password database, which happens all the time). So, while that’s not about remote code execution, I will agree that what is actually logged in this SQLite database could be important information. But I would expect a little more than “OMG! They’re logging to a SQLite database, and you could log bad things to a SQLite database!”
    - xmodem 7y ago
      Where there's smoke there's fire. * Bad SQLite queries can cause code execution * Zoom is using concatenation to build queries * Zoom uses SQLite to store message history and other remotely-controllable data I've seen enough. I don't need to see the entire chain - which in this case would be that the message history queries specifically are built in an unsafe way - to know that Zoom is software that I absolutely should not trust, and that I do not want to have installed on any of my endpoints if I want the other data on them to remain secure. Unfortunately, I do not have a choice. I am working with two telehealth providers who use Zoom. So the next best thing is to be as angry as possible about it. The hiring of Alex Stamos is a good move but we need to keep the pressure on. Put another way, I work for an EU-based fintech. We have similar guidelines (plus a strict log rotation policy, partially due to GDPR but also for many other reasons), but i'm pretty sure our head of security would shit bricks if he found out that our logging framework had an RCE.

by Pxtl 7y ago

bitexploder 7y ago

For every person that gets SQL strong concatenation right 20 get it wrong. And there are simple and safe ways to write arbitrary SQL queries without using concatenation. Parmeterized queries are available everywhere. There is a reason we tell everyone not to do it and that reason is that it’s dangerous and almost everyone screws it up. I have found your case to be the exception in 13 years as an infosec researcher and consultant.

shakna 7y ago

Building SQL queries from strings is possible safely, in the same way that C's memory model is safe. It can be done... But not by most people. Are history proves it, time [0] after time [1] and time [2] after time [3]. It is simply not worth it, if you can make use of parameterized queries. [0] https://web.archive.org/web/20161024090111/https://ico.org.uk/about-the-ico/news-and-events/news-and-blogs/2016/10/talktalk-gets-record-400-000-fine-for-failing-to-prevent-october-2015-attack/ https://web.archive.org/web/20161024090111/https://ico.org.u... [1] https://www.nytimes.com/2014/08/06/technology/russian-gang-said-to-amass-more-than-a-billion-stolen-internet-credentials.html?_r=0 https://www.nytimes.com/2014/08/06/technology/russian-gang-s... [2] https://www.baltimoresun.com/education/bs-xpm-2014-03-07-bs-md-hopkins-servers-hacked-20140306-story.html https://www.baltimoresun.com/education/bs-xpm-2014-03-07-bs-... [3] https://web.archive.org/web/20150219165019/http://www.batblue.com/united-nations-internet-governance-forum-breached/ https://web.archive.org/web/20150219165019/http://www.batblu...

jackewiehose 7y ago
> Building SQL queries from strings is possible safely, in the same way that C's memory model is safe. But still you wouldn't say that every software written in C is automatically insecure. You have to look into the actual case. Just grepping for SQL-string-concatination or uses of sprintf doesn't say much.
- Grollicus 7y ago
  The difference is that there are sometimes no real alternatives to doing memory management by hand - which means there are prople that are really really really good at this. With sql that's different, every sane sql client library supports some form of prepared statements out of the box and they are really simple to use. There is no reason whatsoever to do this by hand, except for people that don't know better.
  - Pxtl 7y ago
    > every sane sql client library supports some form of prepared statements out of the box this is true > they are really simple to use this is not. The amount of boilerplate I've written to properly parameterize a common pattern like SELECT * FROM FOO WHERE FOO.SomeKey IN (/* user-provided list of chosen values goes here */) is grotesque.
    - pdkl95 7y ago
      I'm not sure I would classify a sql client library that doesn't support arrays as "sane". This may have been annoying in the past, I would expect modern libraries to handle basic features like 'IN" automagically. list = ['bar', 'baz', "Robert'); DROP TABLE Students;--"] Foo.where(name: list) # Foo Load (0.3ms) # SELECT "foo".* FROM "foo" # WHERE "foo"."name" IN (?, ?, ?) # [["name", "bar"], # ["name", "baz"], # ["name", "Robert'); DROP TABLE Students;--"]]
  - ufmace 7y ago
    I disagree with the part that there is always an alternative to building strings for SQL statements. Yes, all client libraries support prepared statements. But none of them support placing prepared statement fields in any part of the SQL statement. Prepared statements work fine as long as you just have simple SELECT statements with a few WHERE values as parameters. They completely fail if you need to do any advanced SQL with anything dynamic. Like optionally add more sophisticated calculated properties to the SELECT fields, conditionally JOIN in extra tables, use any of the DB engine's more advanced XML or JSON parsing features in a conditional way, support choosing <, > or =, etc.

EvanAnderson 7y ago

Setting aside the security ramifications of getting SQL string concatenation wrong, your RDBMS query planner may be able to cache a paramaterized query plan, whereas a query built by string concatenation may be seen as a new query to be compiled and optimized.

Pxtl 7y ago
Maybe it's just Microsoft's SQL server, but I've come to become extremely suspicious of this. I've run into far too many cases where, despite our DBA playing with cache-clearing strategy and statistics and the like, we found that queries would run fine in the unparameterized query-console and would run with pathological performance in the actual application. Every time it's "hey mr senior developer, I'm trying to reproduce that slow query but I can't, even though I'm doing the same thing as the application" and I'm like "you must be doing something wrong" and sure enough: no, it's the SQL server doing something wrong. And every time we found out "oh, running it un-parameterized makes it fast". So then I have to talk down the juniors who are like "well fine, I can fix it by running it unparameterized, let's just do that!". So then we have to figure out how to trick it to generating a non-idiotic query plan without abandoning the security that parameterization provides. ... basically I'm deep into "the emperor has no clothes" on MS SQL server in general.
- bcrosby95 7y ago
  This is like a window into my current problems. We've never used SQL Server, but a vendor for a client requires it. We are constantly running into this problem. Right now we have a query that takes 300ms when we back out the parameters, or ~12 minutes when we parameterize it. It is extremely frustrating. Never ran into problems like this with Postgres or MySQL.

maxlybbert 7y ago

I agree. SQL string concatenation that combines programmer-controlled strings with potentially-user-controlled numbers, for instance, is immune from SQL injection. And it’s a connection to a local SQLite database for session data and configuration info, so SQL injection wouldn’t really be able to do much (later the report says the database has history, logs, and “probably also sensitive data such as passwords” but doesn’t say what evidence there is that there’s sensitive data in the database). The sprintf example acknowledges that it’s not clear if the strings involved are user-controlled. It’s definitely possible that the programmer knows all the strings that may be involved and can prove that buffer overflow is impossible. There’s the complaint that Zoom crash reporting reads registry values that the application has permission to read. This is only problematic because it sends that information to Zoom’s crash reporting service; i.e., somebody other than the user. But programs if the program can’t write sensitive data to the registry, then there’s nothing sensitive to disclose.

xmodem 7y ago
Take a look at this defcon talk: https://www.youtube.com/watch?v=JokZUjwGj4M https://www.youtube.com/watch?v=JokZUjwGj4M If message history goes into that database and is not sanitised properly there's a good chance this gives other conference participants RCE.
- maxlybbert 7y ago
  I work at a bank, and we have strict rules about what data can be logged (and what data can never be logged), to avoid accidental information disclosure if somebody ever gets our logs (which, I assume, would be about as hard to do as getting somebody’s password database, which happens all the time). So, while that’s not about remote code execution, I will agree that what is actually logged in this SQLite database could be important information. But I would expect a little more than “OMG! They’re logging to a SQLite database, and you could log bad things to a SQLite database!”
  - xmodem 7y ago
    Where there's smoke there's fire. * Bad SQLite queries can cause code execution * Zoom is using concatenation to build queries * Zoom uses SQLite to store message history and other remotely-controllable data I've seen enough. I don't need to see the entire chain - which in this case would be that the message history queries specifically are built in an unsafe way - to know that Zoom is software that I absolutely should not trust, and that I do not want to have installed on any of my endpoints if I want the other data on them to remain secure. Unfortunately, I do not have a choice. I am working with two telehealth providers who use Zoom. So the next best thing is to be as angry as possible about it. The hiring of Alex Stamos is a good move but we need to keep the pressure on. Put another way, I work for an EU-based fintech. We have similar guidelines (plus a strict log rotation policy, partially due to GDPR but also for many other reasons), but i'm pretty sure our head of security would shit bricks if he found out that our logging framework had an RCE.