4 ms·

Glad to see the communication between Orange and GitHub went great. Both did the right thing after discovering the flaw. Orange reported it and GitHub fixed it

by sytse 10y ago

Glad to see the communication between Orange and GitHub went great. Both did the right thing after discovering the flaw. Orange reported it and GitHub fixed it and allowed Orange to publish a blog post. I have to admit I'm a bit of a fanboy of HackerOne. The custom SQL query is something that is hard to prevent. It is still hard to use ActiveRecord/Arel for everything. I'm sure GitLab is not immune to it. GitHub chooses to encrypt their source code to prevent modifications. Our experience at GitLab is that customer modifications don't cause a lot of extra load on our support team. But of course that might be caused by having different architectures and customers.

BinaryIdiot 10y ago
> GitHub chooses to encrypt their source code to prevent modifications. Our experience at GitLab is that customer modifications don't cause a lot of extra load on our support team. But of course that might be caused by having different architectures and customers. Most of the companies I have worked at that require code obfuscation and / or encryption do so because they believe someone will steal their source and re-use it or at least discover how it works and re-implement it in an attempt to steal customers (typically they suggest China in all of those). I'm not sure what GitHub's reasoning is but I hadn't heard of someone doing it to prevent additional support load. That's an interesting an angle I hadn't thought of.
- mcintyre1994 10y ago
  Github use a key for their obfuscation : "This obfuscation is intended to discourage GitHub Enterprise customers from making modifications to the VM. We know this 'encryption' is easily broken." So that's probably why he assumed their reasoning was to do with reducing support by avoiding modifications by customers.
  - marcinkuzminski 10y ago
    Interesting, from our point of view at RhodeCode, allowing modification of code often benefited our support team. When testing some real edge cases, we were able to inject additional debug code to pinpoint the problems. It actually helps IMHO. Furthermore ability to add a patch to the code to fix a bug within minutes could be a selling point for some types of customers.
    - BinaryIdiot 10y ago
      After doing support for folks who used code I developed where they modified it without telling me, I could see the merit of both ways. I've spent days remotely debugging an issue before realizing a customers actually did modify our product when they kept insisting it was never modified (he had no idea). So I could see it working out both ways. I know one of the companies I dealt with who obfuscated their code could send us a debug version for testing / debugging a specific issue then we'd revert back to the release version. It's a little more complex but I kinda like that model. But as you said it works great for you :)
qaq 10y ago
It's amazingly easy to prevent for high value target type web apps use stored procedures only. The web app db user should only have permissions to execute appropriate stored procedures.
- koolba 10y ago
  Stored procedures aren't a panacea for SQL injection. The real answer is to use parameters for any user input. Otherwise if you're concatenating strings to invoke your stored proecedures you're in the same boat.
  - kodablah 10y ago
    The sad part is many libs force you to accept prepared statements with your parameters or you get no parameters at all.
    - ThisIs_MyName 10y ago
      Care to elaborate?
  - qaq 10y ago
    It's extremely easy to validate in automatic way that only proper call's are used to call the sprocs also if you limit web app db user to only have permission to execute appropriate stored procedures you mitigate a good number of attacks even if someone did call the sproc in unsafe manner that allows sql injection now the attacker needs to guess sproc names and signatures and still limited in what he/she can do. This assumes that sprocs are themselves not written to concat sql and then execute it :)
  - justinclift 10y ago
    As a side note, it's not always possible to use parameter binding for all parts of a SQL query. :( For example, when querying user uploaded databases (not a typical use case), the SQL query needs to include the table name. At least with SQLite (unsure about PostgreSQL), table names can't be bound in a parameter. They need to be part of the non parameterised query string, thus string smashing. Field/column names are probably similar too, though I haven't checked (yet).

by sytse 10y ago

BinaryIdiot 10y ago

> GitHub chooses to encrypt their source code to prevent modifications. Our experience at GitLab is that customer modifications don't cause a lot of extra load on our support team. But of course that might be caused by having different architectures and customers. Most of the companies I have worked at that require code obfuscation and / or encryption do so because they believe someone will steal their source and re-use it or at least discover how it works and re-implement it in an attempt to steal customers (typically they suggest China in all of those). I'm not sure what GitHub's reasoning is but I hadn't heard of someone doing it to prevent additional support load. That's an interesting an angle I hadn't thought of.

mcintyre1994 10y ago
Github use a key for their obfuscation : "This obfuscation is intended to discourage GitHub Enterprise customers from making modifications to the VM. We know this 'encryption' is easily broken." So that's probably why he assumed their reasoning was to do with reducing support by avoiding modifications by customers.
- marcinkuzminski 10y ago
  Interesting, from our point of view at RhodeCode, allowing modification of code often benefited our support team. When testing some real edge cases, we were able to inject additional debug code to pinpoint the problems. It actually helps IMHO. Furthermore ability to add a patch to the code to fix a bug within minutes could be a selling point for some types of customers.
  - BinaryIdiot 10y ago
    After doing support for folks who used code I developed where they modified it without telling me, I could see the merit of both ways. I've spent days remotely debugging an issue before realizing a customers actually did modify our product when they kept insisting it was never modified (he had no idea). So I could see it working out both ways. I know one of the companies I dealt with who obfuscated their code could send us a debug version for testing / debugging a specific issue then we'd revert back to the release version. It's a little more complex but I kinda like that model. But as you said it works great for you :)

qaq 10y ago

It's amazingly easy to prevent for high value target type web apps use stored procedures only. The web app db user should only have permissions to execute appropriate stored procedures.

koolba 10y ago
Stored procedures aren't a panacea for SQL injection. The real answer is to use parameters for any user input. Otherwise if you're concatenating strings to invoke your stored proecedures you're in the same boat.
- kodablah 10y ago
  The sad part is many libs force you to accept prepared statements with your parameters or you get no parameters at all.
  - ThisIs_MyName 10y ago
    Care to elaborate?
- qaq 10y ago
  It's extremely easy to validate in automatic way that only proper call's are used to call the sprocs also if you limit web app db user to only have permission to execute appropriate stored procedures you mitigate a good number of attacks even if someone did call the sproc in unsafe manner that allows sql injection now the attacker needs to guess sproc names and signatures and still limited in what he/she can do. This assumes that sprocs are themselves not written to concat sql and then execute it :)
- justinclift 10y ago
  As a side note, it's not always possible to use parameter binding for all parts of a SQL query. :( For example, when querying user uploaded databases (not a typical use case), the SQL query needs to include the table name. At least with SQLite (unsure about PostgreSQL), table names can't be bound in a parameter. They need to be part of the non parameterised query string, thus string smashing. Field/column names are probably similar too, though I haven't checked (yet).