@@eslam3kl538 both the writeup link you gave on your tweet and the one LiveOverflow put in the video description do not work because they are missing a "." at the end of it, btw.
Neat explanation, I was so confused when I first saw you tweet about it - I figured it was going to be some weird prototype pollution or something. Props to you for reaching out and looking into this with Eslam instead of just writing this off as bs, and props to Eslam for updating the blog with this info too so others can make use of it!
My first guess was that they implemented their auth function to put its result into the input data structure. One would think that would clash with the valid keys in there, but if the auth function is written as a transformer, replacing the parameters with the result, it would work. This is a common pattern in enterprise applications, where the same data structure needs to be given to a dynamic list of functions (e.g. supplied by plugins) that may all modify it. However, starting with an uncontrolled structure always is a bad idea.
At 11:17 you should read the note just below. The third argument tells CodeIgniter not to escape the "fields", those "fields" are the keys in the JSON object. You're passing 0 as the third argument, which is falsy.
Either that parameter should default to true, or they should have reversed the sense of it. Having to explicitly set a flag to protect against sql injection feels very "php-like" to me.
I wonder whether the backend developer assumed all requests would be well-formed and coming from the frontend, and forgot that they could ever get malformed input?
I literally had to remind my chief architect that was possible. He hadn’t even considered someone using curl or some other non-front end access point to the rest of the application. My mind was blown.
@@airman122469 It is a very common thing I explain to a lot of developers - somehow people think that the browser is a security boundary, and then I show off Burp Intruder and jaws drop. I've even had security people have that reaction; some of them "intellectually were aware" but did not realize what it meant as they hadn't actually seen it demoed, just learned the fact for a CISSP or the like.
I must say, in all honesty, you are the best content creator for all things cyber security. I love how you showcase your thought process and really explain issues and concepts in a very clear and understandable matter. With every video you upload it gives me much motivation to stop being lazy and learn new things every day!
Hey LiveOverflow! Looks like you have either a typo in the link to the new blogpost, or the link was updated. It leads to a 404 website, and the actual blog post has a period at the end of the link. Great deeper dive, by the way! I really appreciate when any kind of bug is analyzed more into depth to find the actual root cause, rather than just shrugging off weird behavior and implementing a bandaid fix - this goes for all forms of programming, debugging, and pentesting. Finding the root cause can end up eliminating many more related bugs and prevent more of the same type in the future, while a bandaid fix just adds to the spaghettiness of code and ultimately it becomes undebuggable.
Directly using the $request->getJSON() in a query builder is a common mistake, I think. It's something similar to using $request->all() in Laravel where its considered an unsafe practice.
Its essentially not sanitizing data. Sure it might be valid json, but you must verify the structure of the json and the contents. This would not have happened if the data was verified before use.
@@ryangrogan6839 Yeah, one thing I keep reminding myself is to never easily trust anything from the frontend, always validate and verify in the backend.
@@xray8863 I would go a step further and not trust ANY data that comes from the outside, regardless of front or back end. If an attacker gets a foothold in any of your services, malicious json can be sent to clients, other services, etc. Always sanitize before you utilize.
Why this is not common sense amongst backend developers? I mean, it's the same as at home leaving your window next to the front door wide open as everyone supposed to go through the front door. Your backend is a public service and your front-end code is open-source, there is not even a security by obscurity element in the whole thing!
These three steps should be mandatory when accepting JSON input: 1. Check if JSON is formatted correctly 2. Check the existence of the keys you want 3. Parse and verify the key values
so it all comes back to simple validate and sanitize unknown inputs.... the very basic of IT security... by the way: the reason it could be benefitial to not only validate for required data but also checking if there is too much data is, that you can monitor possible bugs, but more important you can monitor attacks. if your application always sends username and password und now your app received a completely different payload this may be a very seriois bug in your application or someone is pentesting your app. and to know whether your application is under attack is a valie information, especially if you can live monitor the requests and outcoming results.
It's amazing to me that, in 2023, this is still something programmers end up implementing themselves (and sometimes make mistakes in). Authentication should be a solved problem that frameworks have implemented completely and flawlessly. We really are still in the infancy of computing if this is still the kind of mistake programmers *can* make.
@@fonzanedelungini What does enterprise really mean? That it's "bespoke" or "better" in some undefined way, or that it's being developed by a single company for profit, probably closed-source? Because if it's the latter, I'm fairly certain that this is what causes those kinds of problems.
It is a solved problem and many frameworks have authentication built in and you can also decide to use an authentication provider(facebook or google) but in many projects i worked in there was a real need to customize authentication. And to be fair the bug found in the video is a bug that could happen anywhere really
As per the first example with the if statement, if you just did a not check on both those values and returned a 400, you'd be following basic practices and the invalid requests would have been mitigated. But for the sql injection, if you don't directly pass the json and actually validate the data, then bases are covered. Both of these things should be basic security practice for any API... Easy way to find badly written apps lol so nice find! Thanks for going to find more info about the tweet!
my first guess was that there is a JSON library where "root" is used to get the root of the JSON tree, but it's used in a way where it could also mean the propery "root" in the tree. The equivalent of naming a file ".." in a directory tree.
Been doing PHP for a bit over 5 years now using Laravel mainly and I'd say from my experience at least, ensuring set column names don't come from user input, or if they do ensuring they're validated to a specific set of values, is a pretty common thing. While I've never particularly run into that issue, it has come up in some frameworks in the past, generally in relation to sorting when creating an ORDER BY clause in SQL. In Laravel 5.8 they added an explicit check to the orderBy function on their ORM and Query Builder that checked to make sure the direction parameter was either "asc" or "desc" as a security measure to protect users, and there's a warning in their documentation stating that you shouldn't take in user input for the order by function due to PDO lacking the ability to bind column names.
Here's another "probably": The application is probably storing plaintext credentials. Think about it - if you're passing getJSON with a plaintext password into a getWhere, where is the hashing taking place? It's not happening on the client side based on the demo. If the app was hashing the password from the user, it would probably error out at that point rather than fall through
Hashing client-side is almost as bad as not hashing: If someone pwns your database they can just write a client that sends in the hashes they stole without any further modification. Stolen credentials from other databases work in your app the same as before. HTTPS takes care of protecting the password in transit. The only thing you've protected against (compared to storing in plain text) is leaking a password that is used on another site. Not negligible, but not enough either.
If the server passes raw JSON from the request into the query builder, there's no need for SQL injection. Nothing is stopping you from removing the password field and logging in with just a user name...
This! It's actually more powerful to send `{"username": "root"}` than just `{}` since this will ensure getting the root credentials vs hoping first row returned would be super user
Brilliant! I found the article yesterday when you tweeted about the upcoming video, but still couldn't make any sense of it. Weird that he's mentioning working with you, but still providing a completely different (and inaccurate imo) analysis.
This is something that can happen in any framework and in any language. This isn't mentioned in most framework documentation because it should be common sense to not pass in the entire request to your database queries lol. Although I do that in my personal projects too... it's just such a simple way of making an API. most endpoints are one line of code. But never do that in user management sheesh
The Codeigniter documentation actually warns against this, but in a different user guide. The problem here and why it's not so obivous/common sense is that it does filter values so you'd expect it to filter keys too. And when you do half of the job you need to ensure to make it very clear to the developers that it does not do the other half of the job.
I disagree, it's reasonable assume it would also filter keys even if you're aware of the dangers of not doing so. Why have a framework if it's just going to cause you more trouble? If you have to go looking for the part where the docs warn you about this, then it's just not good. It should be clear as soon as you look up the usage.
Stop trying to excuse the bad code by blaming the user. An ORM that doesn't sanitize parameters should get killed off ; getWhere() is supposed to sanitize the inputs, there's no reason it shouldn't sanitize the keys. If programmers like you were right we'd still be stuck in the 90s with SQL injections everywhere "Guardrails?! If the mason saws off his fingers it's because he was a BAD mason!" Cool opinion, my bandsaw has guardrails.
@@Wyvernnnn I'm not a framework developer, but the reason of not sanitizing the keys is very simple: you can't. If the user can control the keys he can construct any query he likes without SQL injections. Instead of {"username": "user", "password": "pass"} he could literally send {"id": "1"} and probably log in as admin. There is no way for the framework to sanitize this because there is nothing to sanitize here.
Just like in hunting a bug, once you find something that works, you have to modify it to see when it stops working! So if "root":"blabla" works, I'd first start by "blabla2" and then by "root2", then by deleting it altogether, just to see if the code is even reading that. The initial conclusion really felt like superstitious thinking observed in pigeons associating a button that does nothing with whether the random food gets served or not.
One of my proudest pwns was figuring out that like half of all LDAP integrations on the web can basically be null byte injected in the password field and you get login, I figured this out because 90% of ldap setups allows for "anonymous" logins in order to get basic info about the user, but many web devs just check if the user is authenticated, so if you can somehow initiate a login with a zero length password you get access.. Only downside is that you need to know a user name. I found this at a company I worked for about 8 years ago, and doing a little but of research it seems like we were not the only ones with that mistake. Especially on PHP sites, someone might check the length og $password, but if you can null byte inject the LDAP interface, then a password length is pretty useless.
This reminds me of the sequelize ORM in NodeJS. They had this query syntax based in objects, almost looked like ast for a query, for example {"$or": [{"type": 3}, {"group": {"$lt": 5}]} Would produce WHERE 'type' = 3 OR 'group' < 5 And applications tended to have a vulnerability when users were able to pass objects as user input. So eventually they replaced it so that operators have to be Symbol keys, which can't be created by the user. {[Op.or]: [{"type": 3}, {"group": {[Op.lt]: 5}]} Where Op is an object provided by the library containing a bunch of Symbol values
hey, senior webdev here. i dont do much with php nowadays, because i value my mental health, but i worked on many php based projects in the past. to me personally, the auth bypass issue seems to be such a simple programmer error and so detached from the used framework, that it doesnt make much sense to include this in any framework-specific documentation. if the code really looks like this, it is just a really careless oversight and you cant include all of the ways to do things wrong. it would only be correct to blame the frameworks if they happen to have a kind of tutorial that already comes with this code as a starting point. in my php time i noticed that the php community is very prone to just copy-paste security relevant code without questioning it, so this might have lead to the issue. the sql injection issue is cool though, because it gives a clue on what ORM the devs used and potentially also what version. if the documentation of that ORM doesnt warn users that the keys are not escaped, then this is a pretty big oversight that can lead to many more devs to run head first into that trap ^^
It's crazy how it's always PHP when such obscure issues arise. Not saying this doesn't happen in other languages as well, but the paradigm of "just pass us the data and we'll handle it" just always reminds me of this language.
PHP powers most of the web, so it's going to be in the firing lines for this sort of thing a lot more. That said, there's literally nothing about this issue that is PHP specific.
This is why you should validate this stuff at multiple levels, for example with validation middlewares and filters before even allowing it to go into a controller. Their client had 0 attention to detail...
This is quite simple. If you find a site which this login works please notify the owner that the developer should NOT be writing code for public consumption. One: line 20 SHOULD simply return an error if either username or password is missing. Ideally, it barfs if any other object is there. But minimally, you should validate required fields are present and ignore extraneous. I know your not a dev, but neither is the person writing the site under investigation. Second, taking parameters from the keys of the input is a for, of begging to be hacked. But if one doesn’t know better than the first issue, they don’t know what SQL injection is, let alone how to prevent it.
just remembred this video again. this format is amazing! would love if you would make it into a sieris. extremley interesting, straight to the point and learned alot :D
Cool find, had a hunch straight away "root" was from messing with content mime types between JSON & XML and hoping the response would spit out some parser clues or possible XXE as you mentioned. 1:58 Looking forward to this video. I generally use it lazily to see if clientside JS attempts to redirect me or expose endpoints I should look at manually. I know know, get gud at scraping JS for such endpoints.
I find this insanely important, to somehow reverse engineer a technique where you are talking to, but have no profound understanding. For instance: I discovered how a Microsoft cloud application used WebSockets with GZIP instead of traditional HTTP requests to transfer data to its end users. This is how I now interpret performance issues with the server. Having an understanding of a framework is just utterly important to have a basic foundation of problem solving.
What really grinds my gears is that there is no mechanism to check the password in code. It’being done on the database level in the where clause. Now I know this is just an example liveOverflow used, but judging by the behavior of the target website it’s entirety possible they also use plain text passwords, which is another level of issues altogether. They should’ve used something like BCrypt or Argon2ID both of which are supported fully by PHP.
The plain-text password should not have left the client side in the first place. Hashing with a nonce would both prevent a stolen ciphertext replay attack and having to send the client data straight to the db without doing something with it.
@@tranquility6358 Yeah, IF everything is working as intended, then it is not a problem. However, if something goes wrong, then it is a huge weakness. For example, if the server can be accessed on a non-TLS connection (due to configuration oversight, either now, or someone makes a mistake in the future) and an attacker can somehow forcibly degrade the client's connetion, then it can steal the password. Or, if an attacker can gain access to the user database, it can aquire everyone's password which can be used to impersonate people on other platforms as well. Serious issue, as we know that a lot of people DO reuse the same password on other platforms. Security is not one level. It is multiple layers on top of each other. If one fails, the others can still prevent or mitigate the problem.
I hate query builders. SQL is already readable, QBs are redundant and don't really provide much; plus you have to relearn how to do things in whichever QB you're using. You end up making several DB calls in the back end if your DB is on a separate server and the migration features are not as useful as their creators like to pretend. Now you show that the keys aren't parameterized, which is just embarrassing. I can't find any case where anyone should be using QBs.
I have one case for partial QBs: Compatibility with multiple database engines. On the other hand, scattering SQL all over the code creates a maintenance nightmare. QBs are one way to fix this, although not the best one. But any other would require the programmer to actually think about the structure of their program upfront. Companies may even be forced to pay an IT architect instead of throwing an intern at the problem!
I also saw the same tweet on Twitter recently and I was scratching my head for a second... I'm curious about Twitter's algorithm now, considering that post was from a year ago.
Sometimes when I have time in web app pentesting, I deliberately try "weirdly different". That's what fuzzing is in general, but I mean probes that are in a way unjustified that are sort of off the wall just to see if anything unusual happens. This is unsustainable and slow but can sometimes be interesting in results.
PDO (the most common driver for database communication in PHP) does not support binding to columns, you can only bind to parameters - passing any user input to any framework’s where clause (in the column field) will cause a SQLi - unfortunately, it isn’t well known as far as I’ve seen
It wild that parameterised queries aren't as well known as they are. In a lot of other languages you kinda have to do it that way or you really have to go out of your way to introduce an SQL injection vunerability.
moment I saw root, I knew it was XML XD, but what type of auth for which backend services? I feel like, first instinct for if root or random XML working would be to check if any parameters or empty object worked too. edit: watched more of the video, as expected, this is from php... codeigniter query builder. overall, interesting video, and thanks for sharing the journey.
modifying the response is basically _"pray the client side code has oversights in it that will let you do things on the server you should not be able to anyway"_ and unfortunately for the webmasters, it does sometimes work...
@@codegeek98 Nah. That is a return response, and NOT a root anything. You'll also notice the response is still a JSON Object, not an array. His code is mere speculation, and incorrect at that.
It seems to me the part where the keys of the array passed to getWhere() are not escaped IS a bug in CodeIgniter, not just a mistake made by developers using Code Igniter like the other cases
The moral of this story for a practising programmer is: always, always aggressively validate user input and return 400 Bad Request if anything is even slightly off. Passing user-submitted JSON directly into a query builder is a terrible idea. JSON schema is a thing that exists, so one could automate validation to a large extent, but even ad-hoc validation can be leagues better than nothing at all. And before someone says that "frameworks should take care of it", good point, but don't blindly trust frameworks to always do the Right Thing, either verify user data yourself or be absolutely sure your framework does it for you -- in my experience, most don't, or at least don't do a terrific job of it, because only you know what kind of data your application expects.
As a developer, it takes minimal effort to check if data is not null nor empty and escaped it properly before sending to DB. They deserve to be hacked if such basic safeguards are not implemented. Pretty sure their codebase is full of vulnerabilities and needs to be fully audited.
Interesting, that is not what I thought the cause would ultimately be. I thought it would be one of two things 1) It was a bug where during deserialization there was an object called "root" that was being overwritten. 2) There was a separate login page for admin users that wrapped the credentials in the root object as a signal to the authentication script that the user should get higher level access (yes I have seen that before). I'm genuinely shocked it was something that simple though. Sounds like another case of people just mindlessly copying code without understanding it.
turning parameters into save sql is pretty easy: you jsut work with prepared statments but as far as i know column named cannot be easily turned into prepared statement format (this is the main reason for why i build my database npm layer with a somewhat annoying querybuilder)
I never had such kinds of mistakes.... I hope I continue that way. For me, these stick out like a sore thumb. If username or password are not provided, just jump out of the function with a generic error that either was not provided.
There's unfortunately no uniform way to escape column or table names in SQL database implementations. MySQL and MariaDB use backticks, Postgres uses double quotes (which I believe is also the SQL standard way) and SQL Server uses square brackets. This seems to result in these sort of "injection due to unescaped column name" issues. To me it seems like something that needs fixing in CI to escape column names and table names depending on what DBMS is used.
While that is true, I see the root cause more in the desire to make SQL builders as dumb as possible. Instead of querying the database for column names and matching the parameters to those (throwing an error for unmatched ones), they just convert the parameter into text. However, a builder like that is useless; writing the where clause manually is arguably easier than building the data structure to feed to it---and it involves a skill that will stay valid forever instead of being framework-specific. I much prefer to write select("select * from users where username=? and password=?", $username, $password) over select({username => $username, password => $password}, "users") oops, select("users", {username => $username, password => $password}) still wrong, select({username => $username, password => $password, table => "users"}) maybe?.
@Gunni the problem is that when you use a framework, especially one that says it cleans up your values, it's reasonable to expect that it also would escape the names as well. It creates a certain expectation from developers, and when it doesn't deliver (either by escaping names or explicitly warning that it doesn't), it will end up causing issues. This is especially true if other frameworks do actually escape names by default (most do AFAIK).
@@ParodieHecker-mobile nope. the guy’s name is Islam in Arabic that’s اسلام and he’s Egyptian like me which I found cool as I’ve never seen an Egyptian featured on this channel before. In Egypt when we want to tell someone that he did a good job we say Allah Yenawar الله ينور which translates in meaning to good job shedding light on this. Next time when using Google translate understand that just because it says it can translate Arabic doesn’t mean it’s correct. The Arabic language is full of nuances and has a lot of dialects that stray very far from the traditional Arabic like the Egyptian dialect and slang expressions that to Google will make no sense. Keep that in mind.
Jeez. So my takeaways from this:- 1. Why no check for invalid parameters? A login is invalid if either of the username or password are not specified, I shouldn't even need to go to the database for that one. A simple set of "if !isset" guards and the first issue just won't happen. 2. Why on earth would you ever pass a Json blob directly from your input to your DB abstraction? That seems as weird as the DB abstraction even accepting that nonsense to begin with. Am I unreasonable in thinking that in $currentDecade we really should have well and truly learnt this lesson?
This whole vulnerability (apparently) caused just because somebody did not parsed explicitly for two exact strings, "username" and "pass", and instead dumped the whole thing directly to database that does exactly what the input says. Yeap
This is not a Codeigniter vulnerability though, no? Just seems like bad input validation. You could definitely argue the SQL injection with the dictionary keys is a Codeigniter issue, though.
Even the SQL injection with the keys is not really a CodeIgniter vulnerability since you have to specifically pass an extra parameter to the query builder in order to skip the sanitization (see second note at 11:15)
I suppose because it's not sensible for a developer to inject unvalidated column names from a client straight into a query builder. I find it much weirder that the CodeIgniter docs talk about escaping the values, which indicates they are not using prepared statements. Does anyone know what the reasoning (or history) of this is?
I think because it's not really an issue if you are coding up the controller properly. You should set the keys by hand, not just throw whatever you receive in. Of course, no accounting for devs misusing your framework, and a warning about this in the documentation would be the minimum
how is this a php or codeigniter related? you could easily replicate the same issue with any other query builder, even something more robust like efcore for dotnet
Thank you Eslam for sharing these details with us! Thanks to your openness we were able to learn some really cool stuff!
First. :3
🔥🔥🔥🔥🤝🐀🐁🐭👨💻
I also love the video editing style, so I want to replicate this, and expand on it.
My pleasure to work together bro 🎉
@@eslam3kl538 both the writeup link you gave on your tweet and the one LiveOverflow put in the video description do not work because they are missing a "." at the end of it, btw.
Neat explanation, I was so confused when I first saw you tweet about it - I figured it was going to be some weird prototype pollution or something. Props to you for reaching out and looking into this with Eslam instead of just writing this off as bs, and props to Eslam for updating the blog with this info too so others can make use of it!
yeah, really cool to reach out and follow up! y'all both learned something, and now we get to as well!
My first guess was that they implemented their auth function to put its result into the input data structure. One would think that would clash with the valid keys in there, but if the auth function is written as a transformer, replacing the parameters with the result, it would work.
This is a common pattern in enterprise applications, where the same data structure needs to be given to a dynamic list of functions (e.g. supplied by plugins) that may all modify it. However, starting with an uncontrolled structure always is a bad idea.
what is prototype pollution, in detail, EH.
At 11:17 you should read the note just below. The third argument tells CodeIgniter not to escape the "fields", those "fields" are the keys in the JSON object. You're passing 0 as the third argument, which is falsy.
ohhhhhh I did not realize that! good catch!
Either that parameter should default to true, or they should have reversed the sense of it. Having to explicitly set a flag to protect against sql injection feels very "php-like" to me.
You could still probably send "{id: 1}" and log in to the admin user (or any other user id) since the id field is probably valid
@@msmyrk yeah, if it defaults to falsy it’s very bad
@@msmyrk it does default to true. LiveOverflow is setting it to false (0) in the video.
I wonder whether the backend developer assumed all requests would be well-formed and coming from the frontend, and forgot that they could ever get malformed input?
Exactly!
I'd assume they didn't realize the query builder completely removes the where clause instead of checking "where username is null"
I literally had to remind my chief architect that was possible. He hadn’t even considered someone using curl or some other non-front end access point to the rest of the application. My mind was blown.
@@airman122469 It is a very common thing I explain to a lot of developers - somehow people think that the browser is a security boundary, and then I show off Burp Intruder and jaws drop. I've even had security people have that reaction; some of them "intellectually were aware" but did not realize what it meant as they hadn't actually seen it demoed, just learned the fact for a CISSP or the like.
@@logiciananimal what is CISSP in detail.
I must say, in all honesty, you are the best content creator for all things cyber security. I love how you showcase your thought process and really explain issues and concepts in a very clear and understandable matter. With every video you upload it gives me much motivation to stop being lazy and learn new things every day!
Hey LiveOverflow! Looks like you have either a typo in the link to the new blogpost, or the link was updated. It leads to a 404 website, and the actual blog post has a period at the end of the link.
Great deeper dive, by the way! I really appreciate when any kind of bug is analyzed more into depth to find the actual root cause, rather than just shrugging off weird behavior and implementing a bandaid fix - this goes for all forms of programming, debugging, and pentesting. Finding the root cause can end up eliminating many more related bugs and prevent more of the same type in the future, while a bandaid fix just adds to the spaghettiness of code and ultimately it becomes undebuggable.
Directly using the $request->getJSON() in a query builder is a common mistake, I think. It's something similar to using $request->all() in Laravel where its considered an unsafe practice.
Its essentially not sanitizing data. Sure it might be valid json, but you must verify the structure of the json and the contents. This would not have happened if the data was verified before use.
@@ryangrogan6839 Yeah, one thing I keep reminding myself is to never easily trust anything from the frontend, always validate and verify in the backend.
@@xray8863 I would go a step further and not trust ANY data that comes from the outside, regardless of front or back end. If an attacker gets a foothold in any of your services, malicious json can be sent to clients, other services, etc. Always sanitize before you utilize.
Why this is not common sense amongst backend developers?
I mean, it's the same as at home leaving your window next to the front door wide open as everyone supposed to go through the front door.
Your backend is a public service and
your front-end code is open-source, there is not even a security by obscurity element in the whole thing!
These three steps should be mandatory when accepting JSON input:
1. Check if JSON is formatted correctly
2. Check the existence of the keys you want
3. Parse and verify the key values
normally, I don't care whats in the object, I just take what I need, validate and move on.
so it all comes back to simple validate and sanitize unknown inputs.... the very basic of IT security...
by the way: the reason it could be benefitial to not only validate for required data but also checking if there is too much data is, that you can monitor possible bugs, but more important you can monitor attacks. if your application always sends username and password und now your app received a completely different payload this may be a very seriois bug in your application or someone is pentesting your app. and to know whether your application is under attack is a valie information, especially if you can live monitor the requests and outcoming results.
It's amazing to me that, in 2023, this is still something programmers end up implementing themselves (and sometimes make mistakes in). Authentication should be a solved problem that frameworks have implemented completely and flawlessly. We really are still in the infancy of computing if this is still the kind of mistake programmers *can* make.
It really is hard to believe that something like this is a thing in any enterprise application.
If these developers had evolved to the present times, they wouldn't be using php
@@fonzanedelungini What does enterprise really mean? That it's "bespoke" or "better" in some undefined way, or that it's being developed by a single company for profit, probably closed-source? Because if it's the latter, I'm fairly certain that this is what causes those kinds of problems.
We're still implementing password authentication from scratch today, it's hopeless.
It is a solved problem and many frameworks have authentication built in and you can also decide to use an authentication provider(facebook or google) but in many projects i worked in there was a real need to customize authentication. And to be fair the bug found in the video is a bug that could happen anywhere really
when I saw "ci_session" my mind went instantly to "wait, are they using codeigniter?" lol I used to code with that a few years back
As per the first example with the if statement, if you just did a not check on both those values and returned a 400, you'd be following basic practices and the invalid requests would have been mitigated. But for the sql injection, if you don't directly pass the json and actually validate the data, then bases are covered. Both of these things should be basic security practice for any API... Easy way to find badly written apps lol so nice find! Thanks for going to find more info about the tweet!
my first guess was that there is a JSON library where "root" is used to get the root of the JSON tree, but it's used in a way where it could also mean the propery "root" in the tree.
The equivalent of naming a file ".." in a directory tree.
Haha. Thought the same.
It was awesome. When I saw $where = []; I gave the same reaction when you saw that 'root' came from the xml formatting bug
Been doing PHP for a bit over 5 years now using Laravel mainly and I'd say from my experience at least, ensuring set column names don't come from user input, or if they do ensuring they're validated to a specific set of values, is a pretty common thing.
While I've never particularly run into that issue, it has come up in some frameworks in the past, generally in relation to sorting when creating an ORDER BY clause in SQL.
In Laravel 5.8 they added an explicit check to the orderBy function on their ORM and Query Builder that checked to make sure the direction parameter was either "asc" or "desc" as a security measure to protect users, and there's a warning in their documentation stating that you shouldn't take in user input for the order by function due to PDO lacking the ability to bind column names.
why is it possible to add non standard parameters to orderby in an ORM? isnt an ORM database aware?
Here's another "probably": The application is probably storing plaintext credentials. Think about it - if you're passing getJSON with a plaintext password into a getWhere, where is the hashing taking place? It's not happening on the client side based on the demo. If the app was hashing the password from the user, it would probably error out at that point rather than fall through
Hashing client-side is almost as bad as not hashing:
If someone pwns your database they can just write a client that sends in the hashes they stole without any further modification.
Stolen credentials from other databases work in your app the same as before.
HTTPS takes care of protecting the password in transit.
The only thing you've protected against (compared to storing in plain text) is leaking a password that is used on another site. Not negligible, but not enough either.
@@Reashu I said they are not hashing on the client side
@@patrick1020000 I know, I just wanted to clarify, since you mentioned it, that client-side hashing is hardly better than no hashing.
@@patrick1020000 first of all you should never query for a password as far as I know
If the server passes raw JSON from the request into the query builder, there's no need for SQL injection. Nothing is stopping you from removing the password field and logging in with just a user name...
If they still have something like if (!isset("user") || !isset("pass")) {die;} then it wouldn't work, but that depend on if the dev implemented it
The request worked with an empty body so yes, that is correct
This! It's actually more powerful to send `{"username": "root"}` than just `{}` since this will ensure getting the root credentials vs hoping first row returned would be super user
Why bother loggin in? Can't you just query the information you are looking for? 😄
5:12 "But that's just a theory, a coding theory!" that was 100% not by accident
MatPat reference :P
Brilliant! I found the article yesterday when you tweeted about the upcoming video, but still couldn't make any sense of it. Weird that he's mentioning working with you, but still providing a completely different (and inaccurate imo) analysis.
This is something that can happen in any framework and in any language. This isn't mentioned in most framework documentation because it should be common sense to not pass in the entire request to your database queries lol. Although I do that in my personal projects too... it's just such a simple way of making an API. most endpoints are one line of code. But never do that in user management sheesh
The Codeigniter documentation actually warns against this, but in a different user guide. The problem here and why it's not so obivous/common sense is that it does filter values so you'd expect it to filter keys too. And when you do half of the job you need to ensure to make it very clear to the developers that it does not do the other half of the job.
Oh do you have a link for me where this is mentioned?
I disagree, it's reasonable assume it would also filter keys even if you're aware of the dangers of not doing so. Why have a framework if it's just going to cause you more trouble? If you have to go looking for the part where the docs warn you about this, then it's just not good. It should be clear as soon as you look up the usage.
Stop trying to excuse the bad code by blaming the user. An ORM that doesn't sanitize parameters should get killed off ; getWhere() is supposed to sanitize the inputs, there's no reason it shouldn't sanitize the keys. If programmers like you were right we'd still be stuck in the 90s with SQL injections everywhere
"Guardrails?! If the mason saws off his fingers it's because he was a BAD mason!"
Cool opinion, my bandsaw has guardrails.
@@Wyvernnnn I'm not a framework developer, but the reason of not sanitizing the keys is very simple: you can't. If the user can control the keys he can construct any query he likes without SQL injections. Instead of {"username": "user", "password": "pass"} he could literally send {"id": "1"} and probably log in as admin. There is no way for the framework to sanitize this because there is nothing to sanitize here.
Just like in hunting a bug, once you find something that works, you have to modify it to see when it stops working! So if "root":"blabla" works, I'd first start by "blabla2" and then by "root2", then by deleting it altogether, just to see if the code is even reading that. The initial conclusion really felt like superstitious thinking observed in pigeons associating a button that does nothing with whether the random food gets served or not.
Damn! The way he got deep into the issue is remarkable
One of my proudest pwns was figuring out that like half of all LDAP integrations on the web can basically be null byte injected in the password field and you get login, I figured this out because 90% of ldap setups allows for "anonymous" logins in order to get basic info about the user, but many web devs just check if the user is authenticated, so if you can somehow initiate a login with a zero length password you get access.. Only downside is that you need to know a user name.
I found this at a company I worked for about 8 years ago, and doing a little but of research it seems like we were not the only ones with that mistake. Especially on PHP sites, someone might check the length og $password, but if you can null byte inject the LDAP interface, then a password length is pretty useless.
What happens when the web developer and pen tester both have no idea what they’re doing? “rOoT aRrAy ExPlOiT”
I mean, baby steps. He did a better writeup, looks like he'sjust a beginner and is figuring stuff out.
He's just a newbie, he still found the issues. Understanding comes with wisdom which develops over time.
This reminds me of the sequelize ORM in NodeJS. They had this query syntax based in objects, almost looked like ast for a query, for example
{"$or": [{"type": 3}, {"group": {"$lt": 5}]}
Would produce
WHERE 'type' = 3 OR 'group' < 5
And applications tended to have a vulnerability when users were able to pass objects as user input. So eventually they replaced it so that operators have to be Symbol keys, which can't be created by the user.
{[Op.or]: [{"type": 3}, {"group": {[Op.lt]: 5}]}
Where Op is an object provided by the library containing a bunch of Symbol values
Great! Thanks for sharing you knowledge ❤
hey, senior webdev here. i dont do much with php nowadays, because i value my mental health, but i worked on many php based projects in the past.
to me personally, the auth bypass issue seems to be such a simple programmer error and so detached from the used framework, that it doesnt make much sense to include this in any framework-specific documentation. if the code really looks like this, it is just a really careless oversight and you cant include all of the ways to do things wrong. it would only be correct to blame the frameworks if they happen to have a kind of tutorial that already comes with this code as a starting point. in my php time i noticed that the php community is very prone to just copy-paste security relevant code without questioning it, so this might have lead to the issue.
the sql injection issue is cool though, because it gives a clue on what ORM the devs used and potentially also what version. if the documentation of that ORM doesnt warn users that the keys are not escaped, then this is a pretty big oversight that can lead to many more devs to run head first into that trap ^^
Great video!
Realy liked the deep dive into trying to find out how the code worked even though you didn't have the sources.
And there he is with more awesome, "free" educational videos
Videos like these make me wish I could give multiple upvotes,
Please do a video about the response manipulation topic you mentioned at 2:18
Awesome video. Big thanks to Eslam. Great job 👏
Server: "What is your username and password?"
Hacker: "Yes."
Server: "Welcome back, Sir."
It's crazy how it's always PHP when such obscure issues arise. Not saying this doesn't happen in other languages as well, but the paradigm of "just pass us the data and we'll handle it" just always reminds me of this language.
PHP powers most of the web, so it's going to be in the firing lines for this sort of thing a lot more. That said, there's literally nothing about this issue that is PHP specific.
This is why you should validate this stuff at multiple levels, for example with validation middlewares and filters before even allowing it to go into a controller. Their client had 0 attention to detail...
Validation is usually not up to the developer, but the product owners and they'll usually say no for budget reasons.
@@DoubleOhSilver This. I’ve seen this a lot.
Great Explanation LiveOverflow :) Thanks for digging into that and sharing the outcomes with the community. Also thanks for Eslam. GJ
This is quite simple. If you find a site which this login works please notify the owner that the developer should NOT be writing code for public consumption.
One: line 20 SHOULD simply return an error if either username or password is missing. Ideally, it barfs if any other object is there. But minimally, you should validate required fields are present and ignore extraneous. I know your not a dev, but neither is the person writing the site under investigation.
Second, taking parameters from the keys of the input is a for, of begging to be hacked. But if one doesn’t know better than the first issue, they don’t know what SQL injection is, let alone how to prevent it.
😌 thought so. Felt exactly like XML thing. Didn't thought that it could work by changing JSON key to anything.
just remembred this video again. this format is amazing! would love if you would make it into a sieris. extremley interesting, straight to the point and learned alot :D
Cool find, had a hunch straight away "root" was from messing with content mime types between JSON & XML and hoping the response would spit out some parser clues or possible XXE as you mentioned.
1:58 Looking forward to this video. I generally use it lazily to see if clientside JS attempts to redirect me or expose endpoints I should look at manually. I know know, get gud at scraping JS for such endpoints.
Hey Michael Cera, i didn't know you were a Sec Research guy, loved you on Year One!
Mind-blowing!!! Very very useful and knowledgeable as always!! Thank you! Thank you! Thank you! 🙏💜
I find this insanely important, to somehow reverse engineer a technique where you are talking to, but have no profound understanding.
For instance: I discovered how a Microsoft cloud application used WebSockets with GZIP instead of traditional HTTP requests to transfer data to its end users. This is how I now interpret performance issues with the server.
Having an understanding of a framework is just utterly important to have a basic foundation of problem solving.
What really grinds my gears is that there is no mechanism to check the password in code. It’being done on the database level in the where clause. Now I know this is just an example liveOverflow used, but judging by the behavior of the target website it’s entirety possible they also use plain text passwords, which is another level of issues altogether. They should’ve used something like BCrypt or Argon2ID both of which are supported fully by PHP.
The plain-text password should not have left the client side in the first place.
Hashing with a nonce would both prevent a stolen ciphertext replay attack and having to send the client data straight to the db without doing something with it.
@@gabiold Sending the plain text password over the wire isn’t really an issue as long as the connection is secured using TLS.
@@tranquility6358 Yeah, IF everything is working as intended, then it is not a problem. However, if something goes wrong, then it is a huge weakness. For example, if the server can be accessed on a non-TLS connection (due to configuration oversight, either now, or someone makes a mistake in the future) and an attacker can somehow forcibly degrade the client's connetion, then it can steal the password. Or, if an attacker can gain access to the user database, it can aquire everyone's password which can be used to impersonate people on other platforms as well. Serious issue, as we know that a lot of people DO reuse the same password on other platforms.
Security is not one level. It is multiple layers on top of each other. If one fails, the others can still prevent or mitigate the problem.
Great video
How it looks like => what it looks like
That's why I just love Doctrine.
Good thing my projects are so inconsistent I need to map the front-end keys to different variables anyway 😅
Whoa, what an interesting rabbit hole
Game theory and bob ross references landed hahaha awesome
I hate query builders. SQL is already readable, QBs are redundant and don't really provide much; plus you have to relearn how to do things in whichever QB you're using. You end up making several DB calls in the back end if your DB is on a separate server and the migration features are not as useful as their creators like to pretend.
Now you show that the keys aren't parameterized, which is just embarrassing. I can't find any case where anyone should be using QBs.
I have one case for partial QBs: Compatibility with multiple database engines.
On the other hand, scattering SQL all over the code creates a maintenance nightmare. QBs are one way to fix this, although not the best one. But any other would require the programmer to actually think about the structure of their program upfront. Companies may even be forced to pay an IT architect instead of throwing an intern at the problem!
I'm confused why the word "array" was even used in the first place, as there are no arrays in any of this. "root" is an object, not an array.
ACKHTUALLY objects are just associative arrays of key/value pairs. And arrays are just objects with numbered keys.
@@ra2enjoyer708 JavaScript is a truly terrifying invention.
I saw that pen spinning pen at 4:02 👀
He is a "pen" tester..
I also saw the same tweet on Twitter recently and I was scratching my head for a second...
I'm curious about Twitter's algorithm now, considering that post was from a year ago.
Sometimes when I have time in web app pentesting, I deliberately try "weirdly different". That's what fuzzing is in general, but I mean probes that are in a way unjustified that are sort of off the wall just to see if anything unusual happens. This is unsustainable and slow but can sometimes be interesting in results.
PDO (the most common driver for database communication in PHP) does not support binding to columns, you can only bind to parameters - passing any user input to any framework’s where clause (in the column field) will cause a SQLi - unfortunately, it isn’t well known as far as I’ve seen
It wild that parameterised queries aren't as well known as they are. In a lot of other languages you kinda have to do it that way or you really have to go out of your way to introduce an SQL injection vunerability.
moment I saw root, I knew it was XML XD, but what type of auth for which backend services?
I feel like, first instinct for if root or random XML working would be to check if any parameters or empty object worked too.
edit: watched more of the video, as expected, this is from php... codeigniter query builder.
overall, interesting video, and thanks for sharing the journey.
Dude how does these things even come to you lmao what a GOD !!
modifying the response is basically _"pray the client side code has oversights in it that will let you do things on the server you should not be able to anyway"_
and unfortunately for the webmasters, it does sometimes work...
Not to mention it's not a root ARRAY, but a root OBJECT.
I think the name refers to the server side 9:53
@@codegeek98 Nah. That is a return response, and NOT a root anything. You'll also notice the response is still a JSON Object, not an array. His code is mere speculation, and incorrect at that.
This is why validation is recommended, before you pass json further down your application
It seems to me the part where the keys of the array passed to getWhere() are not escaped IS a bug in CodeIgniter, not just a mistake made by developers using Code Igniter like the other cases
Even if they were escaped you could do like {"username":"blah", "userId":1}
The moral of this story for a practising programmer is: always, always aggressively validate user input and return 400 Bad Request if anything is even slightly off. Passing user-submitted JSON directly into a query builder is a terrible idea. JSON schema is a thing that exists, so one could automate validation to a large extent, but even ad-hoc validation can be leagues better than nothing at all. And before someone says that "frameworks should take care of it", good point, but don't blindly trust frameworks to always do the Right Thing, either verify user data yourself or be absolutely sure your framework does it for you -- in my experience, most don't, or at least don't do a terrific job of it, because only you know what kind of data your application expects.
I like to follow this mantra: don't blacklist faulty inputs, whitelist proper inputs.
And that's why you sanitize, validate and use early returns.
As a developer, it takes minimal effort to check if data is not null nor empty and escaped it properly before sending to DB.
They deserve to be hacked if such basic safeguards are not implemented.
Pretty sure their codebase is full of vulnerabilities and needs to be fully audited.
Can you show the contents of the \Config\Services::request() method? I am having a tough time recreating your example
Interesting, that is not what I thought the cause would ultimately be.
I thought it would be one of two things
1) It was a bug where during deserialization there was an object called "root" that was being overwritten.
2) There was a separate login page for admin users that wrapped the credentials in the root object as a signal to the authentication script that the user should get higher level access (yes I have seen that before).
I'm genuinely shocked it was something that simple though. Sounds like another case of people just mindlessly copying code without understanding it.
turning parameters into save sql is pretty easy: you jsut work with prepared statments
but as far as i know column named cannot be easily turned into prepared statement format
(this is the main reason for why i build my database npm layer with a somewhat annoying querybuilder)
The npm mysql library uses ?? to escape column names. It is still a bad idea to supply the whole user controlled object to the query builder directly
I never had such kinds of mistakes.... I hope I continue that way. For me, these stick out like a sore thumb.
If username or password are not provided, just jump out of the function with a generic error that either was not provided.
There's unfortunately no uniform way to escape column or table names in SQL database implementations. MySQL and MariaDB use backticks, Postgres uses double quotes (which I believe is also the SQL standard way) and SQL Server uses square brackets. This seems to result in these sort of "injection due to unescaped column name" issues. To me it seems like something that needs fixing in CI to escape column names and table names depending on what DBMS is used.
While that is true, I see the root cause more in the desire to make SQL builders as dumb as possible. Instead of querying the database for column names and matching the parameters to those (throwing an error for unmatched ones), they just convert the parameter into text. However, a builder like that is useless; writing the where clause manually is arguably easier than building the data structure to feed to it---and it involves a skill that will stay valid forever instead of being framework-specific. I much prefer to write select("select * from users where username=? and password=?", $username, $password) over select({username => $username, password => $password}, "users") oops, select("users", {username => $username, password => $password}) still wrong, select({username => $username, password => $password, table => "users"}) maybe?.
Say it with me: DO NOT GENERATE QUERIES FROM USER INPUT. Use parameterized queries.
@Gunni the problem is that when you use a framework, especially one that says it cleans up your values, it's reasonable to expect that it also would escape the names as well. It creates a certain expectation from developers, and when it doesn't deliver (either by escaping names or explicitly warning that it doesn't), it will end up causing issues. This is especially true if other frameworks do actually escape names by default (most do AFAIK).
@@yiannissiantos127 I have simply learnt to not trust I prefer to use queries directly
Yeah, what kind of standard is sql? Everyone has their own fucking syntax for sql.
Great writeup! In depth and reproducing the issue
can I ask about the sql payload what is the \" in the payload I don't get it
i love you bro your content is gold !
please keep it up
I usually return with 400 response when the username or password is missing from request body. Why go further
I have seen this very same issue in another custom framework.
01:30 I still don't understand the "array" part. There is no "[" or "]" in JSON data. Where is array?
in the note at 11:17 it says except when using a custom string. maybe "custom string" refers to the key?
5:07 Line 17 should be || and not && otherwise login just fails when both username & password are incorrect but succeeds if one of them is correct.
Always always always sanitize data coming from somewhere else, regardless of if the source is trusted
I like this video. I hope to see more bugbounty ones 😁
عاش يا اسلام، الله ينور 👍🏻👍🏻
Unpassend.
@@ParodieHecker-mobile do you even know what I am saying before saying that it’s inappropriate?
@@DigitalicaEG Yes, you are talking about god/Allah, but this video is about hacking.
@@ParodieHecker-mobile nope. the guy’s name is Islam in Arabic that’s اسلام
and he’s Egyptian like me which I found cool as I’ve never seen an Egyptian featured on this channel before. In Egypt when we want to tell someone that he did a good job we say Allah Yenawar الله ينور which translates in meaning to good job shedding light on this. Next time when using Google translate understand that just because it says it can translate Arabic doesn’t mean it’s correct. The Arabic language is full of nuances and has a lot of dialects that stray very far from the traditional Arabic like the Egyptian dialect and slang expressions that to Google will make no sense. Keep that in mind.
@@DigitalicaEG Hat trotzdem nichts mit dem Video zu tun
Jeez. So my takeaways from this:-
1. Why no check for invalid parameters? A login is invalid if either of the username or password are not specified, I shouldn't even need to go to the database for that one. A simple set of "if !isset" guards and the first issue just won't happen.
2. Why on earth would you ever pass a Json blob directly from your input to your DB abstraction? That seems as weird as the DB abstraction even accepting that nonsense to begin with. Am I unreasonable in thinking that in $currentDecade we really should have well and truly learnt this lesson?
Excellent dig!! 👏
Shoutout to us. 😇
And back in the early 2010s they were saying SQLi is dead
5:12 Nice reference to @FilmTheory :)
Why would you assume that the authentication check code has that mistake?
Isn’t that code that YOU wrote?
Nice game theorists reference
Wow LiveOverflow invented new header "Cintent-Type" (7:17)
This whole vulnerability (apparently) caused just because somebody did not parsed explicitly for two exact strings, "username" and "pass", and instead dumped the whole thing directly to database that does exactly what the input says. Yeap
7:12 It may be obvious I don't use PHP, but that line 15... those backslashes. It just looks so WRONG
This is not a Codeigniter vulnerability though, no? Just seems like bad input validation. You could definitely argue the SQL injection with the dictionary keys is a Codeigniter issue, though.
Even the SQL injection with the keys is not really a CodeIgniter vulnerability since you have to specifically pass an extra parameter to the query builder in order to skip the sanitization (see second note at 11:15)
Wait, isn't CodeIgniter doing prepared statements?
Or maybe that wouldn't help?
Nice explanation, I was really confused before. lol
This is the result of not performing input validation to check for unexpected inputs.
Klassiker. First user is usually the admin.
Now thats some cool content
9:17 I mean.. usually with Automated testing this should also get detected? I would even write Unit-Tests, testing for such edge-cases.
Love this kind of content. Why would a framework deliberately choose to only escape values but not keys tho?
I suppose because it's not sensible for a developer to inject unvalidated column names from a client straight into a query builder. I find it much weirder that the CodeIgniter docs talk about escaping the values, which indicates they are not using prepared statements. Does anyone know what the reasoning (or history) of this is?
I think because it's not really an issue if you are coding up the controller properly. You should set the keys by hand, not just throw whatever you receive in.
Of course, no accounting for devs misusing your framework, and a warning about this in the documentation would be the minimum
thank you dude and plz don't stop
how is this a php or codeigniter related? you could easily replicate the same issue with any other query builder, even something more robust like efcore for dotnet
HAHAH Our entire system is based on CI. Off to work I am!
what is the "root" cause 🥁badumm ts