Hacker News Reader

Comment by sytse | Hacker News Reader

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.

View on HN

> 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.

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.

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.

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 :)

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.

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.

The sad part is many libs force you to accept prepared statements with your parameters or you get no parameters at all.

Care to elaborate?

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 :)

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).

If you're doing anything that esoteric hopefully you understand proper identifier escaping. If not, there's bound to be much worse throughout your app.

That's what libraries like sequel, SQLAlchemy or Jooq are for. I'm looking at implementing some crazy dynamic queries in a CRM system I'm prototyping right now and SQLAlchemy is my saving grace to avoid shooting myself in the foot.

That's true, but an ORM like ActiveRecord ought to protect you from this because you are required to specify valid column names and table names before using it.

It should be able to check if an invalid column or table name is used.

I see Dynamic SQL being used the most for custom queries that require dynamic columns, or especially, dynamic ordering. Using sprocs won't help you there as you still need Dynamic SQL under the hood. I think the best solution is something like Linq-to-Entities with manually defined order-by logic for each query (e.g. an enum defining the sort-order possibilities) or reflection-based matching, neither of which is pretty.

Dynamic SQL is not the problem, you can safely construct queries on runtime in most RDBMS-s, the key thing is to let the DB treat user input as parameters. You can concat strings to create dynamic queries all you want, just don't blindly shove user input in there, construct the query dynamicaly with parameter placeholders and then bind the user-passed input as parameters.

Of course, there's still the matter of those queries not nessarily being syntactically or semantically correct which will only bite you on runtime, but that's a different story.

If you're doing dynamic SQL you either know what you're doing, or don't know that you don't know what you're doing. Although that probably goes for most of things in life.

Reply on news.ycombinator.com