
GitHub Enterprise SQL Injection - urig
http://blog.orange.tw/2017/01/bug-bounty-github-enterprise-sql-injection.html?m=1
======
treehau5
Although I am impressed by the work, I dislike these writeups.

In similar bug bounty posts, a user will post all this brilliant setup code,
these tools I never heard of to decompile binaries, view source, get through
obfuscation, etc.

Then, when you get to the meat and bones, they just go "Ok and vulnurability
is here, and you exploit it like this. Ta data."

It's very anti-climatic, and I don't learn much in the process. I would like
to learn more about what your thought process is like after you get the code.
How did you narrow your focus on ActiveRecord? Did you just know from seeing
it? I would love to learn more. Do expert pen-testers have this checklist of
exploits they try?

Sorry if it sounds like whiny, I would just love it if these had more meat in
the actual finding of the exploit as they did the setup.

~~~
lvh
It's a bit of both. It's not so much a checklist as a classes of bugs, but
close enough. That includes vulnerabilities like SQL injection, command
injection, stored and reflected XSS, et cetera. The WAHH (Web Application
Hacker's Handbook) is a good place to start here; it walks through a lot of
these standard classes of web app vulns.

Something WAHH touches on, and experienced pentesters get a feel for, is more
complex interactions. Often that means chaining several not-so-bad bugs
together to build something really exciting. Sometimes they're just obscure
consequences of say, a rather complex authorization system. These are
typically a lot harder to find, and may involve simultaneous code audits.
Ideally, your long-term in-house security people find these, because any
attacker that does is in the 95th cleverness percentile.

So, TL;DR: start with WAHH; then go find some vulns, and start thinking about
what the app does in terms of alternative things a program could do that a
reasonable programmer who was just trying to get it to work might not have
thought about.

If you're looking for tools to play with, you can do worse than downloading
Kali and running it on a VM. If nothing else, it'll give you a pretty big
catalog of tools to start looking at. I think WAHH covers basic Burp Suite
usage, but Kali has some other tools like sqlmap and BeEF that aren't too
tricky to get started with.

WAHH: [http://mdsec.net/wahh/](http://mdsec.net/wahh/) Kali:
[https://www.kali.org/](https://www.kali.org/)

~~~
chupasaurus
Also want to mention Web Hacking 101 [0], which is written specifically for
newbies in vulnerability searching, with real cases and explanation of how
certain bugs are or could be found.

[0] [https://leanpub.com/web-hacking-101](https://leanpub.com/web-hacking-101)

------
Illniyar
It still boggles my mind that sql injection is a thing in 2016, let alone in a
modern codebase from a great software company with amazing engineers.

Somehow we (the programming profession) are doing this whole thing wrong, and
I'm not sure why.

~~~
nradov
I find it hilarious how people think there are all these "amazing engineers"
around. Sometimes it even seems like hero worship. The reality is that the
vast majority of engineers are mediocre even at high-profile tech companies
and hot startups. That's not a bad thing and I don't mean it as a criticism;
modern civilization has largely been built by mediocre engineers. And even the
best engineers make frequent mistakes.

~~~
tshannon
This sentiment is simultaneously terrifying and comforting.

------
qz_
Props to OP for finding the vulnerability without knowing Ruby on Rails.

~~~
qyv
Interesting that this is a standard use-case for ActiveRecord [0] and does not
include any built-in protection against SQL injection. You can pass a hash of
symbols into the method, but there is no safe way to pass string arguments
with sanitizing them yourself.

[0]
[http://guides.rubyonrails.org/active_record_querying.html#or...](http://guides.rubyonrails.org/active_record_querying.html#ordering)

~~~
andy_ppp
God, IMO this should be reported as a bug in rails; in my opinion it
ActiveRecord should _never_ accept strings for this without parsing them.

~~~
grey-area
This is probably incredibly common for things like order params in many web
apps and also relies on silly conversion rules in MySQL. Any time you see
order=name in params you should be suspicious. Better to use numeric enums.
There are similar problems with column names, this site has some great
examples which apply to many ohms, not just rails:

[http://rails-sqli.org](http://rails-sqli.org)

The lesson is you should _always_ strongly assert the type of user input
(where you can, convert to known good values which is even better), and never
magically convert types as rails/ MySQL does here. It's less convenient but
safer.

~~~
true_religion
Django for example will throw an error if an ordering parameter does not
correspond to a database column.

If you have to specify all the columns of a table in Rails, then ActiveRecord
ought to be able to restrict the order params to only column names.

~~~
smithd98
I imagine some ORMs don't have this so people can do order by computed columns
like "order by assets - liabilities"

To your point it probably makes sense to have an order method that verifies
column names and an order_raw method that takes a string so the caller
understands the risk, while protecting the usual case.

------
tericho
It's hidden way down at the bottom, but I found the Timeline to be my
favourite part :)

Edit: Can't see to format it correctly, just check the original source.

~~~
pyre
This one?

    
    
      2016/12/26 05:48 Report vulnerability to GitHub via HackerOne
      2016/12/26 08:39 GitHub response that have validated issue and are working on a fix.
      2016/12/26 15:48 Provide more vulneraiblity detail.
      2016/12/28 02:44 GitHub response that the fix will included with next release of GitHub Enterprise.
      2017/01/04 06:41 GitHub response that offer $5,000 USD reward.
      2017/01/05 02:37 Asked Is there anything I should concern about if I want to post a blog?
      2017/01/05 03:06 GitHub is very open mind and response that it’s OK!
      2017/01/05 07:06 GitHub Enterprise 2.8.5 released!
    

Or this one?

    
    
      Day 1 - Setting VM
      Day 2 - Setting VM
      Day 3 - Learning Rails by code reviewing
      Day 4 - Learning Rails by code reviewing
      Day 5 - Learning Rails by code reviewing
      Day 6 - Yeah, I found a SQL Injection!

------
lukasm
For those that this website is marked as dangerous
[http://webcache.googleusercontent.com/search?q=cache:z7yyaxI...](http://webcache.googleusercontent.com/search?q=cache:z7yyaxITRoAJ:blog.orange.tw/2017/01/bug-
bounty-github-enterprise-sql-injection.html+&cd=1&hl=en&ct=clnk&gl=uk)

~~~
BinaryIdiot
Is there a specified reason for it being marked as dangerous? It comes up fine
for me. Granted it doesn't support SSL it seems (get that fixed, Orange!) so
maybe you're using a plugin that marks non-SSL as dangerous?

~~~
slipstream-
The website seems to be a blogspot blog on a custom subdomain...

------
mankash666
I'm curious if the author decompiled binaries generated from C code. In that
case, I understand how one could "dis assemble" binaries, but generating
readable C code, is that possible?

~~~
BuuQu9hu
Decompilers exist (like Boomerang), but it is more likely the author used
something like IDA Pro, which has a code flow graph feature.

[http://boomerang.sourceforge.net/](http://boomerang.sourceforge.net/)

~~~
DCoder
Hex-Rays is a pretty good decompiler plugin for IDA Pro: [https://www.hex-
rays.com/products/decompiler/index.shtml](https://www.hex-
rays.com/products/decompiler/index.shtml)

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

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

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

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

