@@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.
@@NootNooot Any code in the wild is far from perfect. Starting at PHP 7.4, PHP can be a type-safe language. The rest of this video is about security by design fail. This is language-independent. You clearly don't use PHP in a stack that has secure information, so you're not up to date with the changes, but I am. You're over generalizing and making yourself look like you don't know what you're talking about to people with knowledge about PHP.
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.
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.
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!
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
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.
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!
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.
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.
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.
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
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.
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!
I mean if your Login Query is "PASSWORD && USERNAME" and if you don´t find a matching entry the password is wrong... Please stop developing Production software... This type of Query is only Possible if the Password ist stored in clear Text... You should never do this. The query should only search for the Username and than compare the Password Hash (like Bcrypt) with the send Password. So maybe this is not a well known issue, because if you have this issue you don´t care about Security in the first place...
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!
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.
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.
If you decide to write such horrible code and not validate user input then you are a bad programmer. It doesn't have to do with a specific framework, codeigniter should have validation like all frameworks, implement it. This is a bug that a junior PHP programmer working with the language more than 6 months should never make
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 ^^
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.
@@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 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)
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...
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.
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 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
11:20 I never facepalmed so hard in my life. This is clearly on Codeigniter. If you tell users that you sanitize input, you sanitize input. Technicalities about keys and values are useless. If they can't fix it, they should alter their doc to push sanitizing onto implementers. Communication is very important.
I would argue that the first example isn't really relevant to the framework - any ORM library would happily allow returning expected rows if the programmer skips passing a "where" clause based on their incorrect logic for detecting parameters - if your implementation is true, it really feels like a programming error and not related to the framework. Plenty of other ORMs would allow the same thing: ``` # Mockup using python alchemy query = User.query() if request.json().get("username") and request.json().get("password"): query = query.where(User.username==request.json().get("username"), password=request.json().get("password") return query.first() ``` The second on the other, as opposed to some other comments, to me, definitely feels like a short-coming of the library. A query method that _should_ accept key-value pairs for fields and values should probably sanitised the field and the value - I'd imagine passing some custom sql, that's designed to append to the where clause, should be a dedicated method of the library (or maybe some attribute to denote a 'raw' value) to avoid this kind of mistake
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.
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?
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.
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
Honestly who writes code like that? This is embarrassingly bad. Just feeding user input into a framework like that. Or fucking up login like that. Beginners surely can do that and it is ok but someone else should be looking over their shoulder and either the company or the 2nd pair of eyes should be ashamed.
Come on, falling for these just shows the limit of the sell made man in computer science. Nowadays, especially if you plan to develop a custom website, these are absolutely mandatory to know about before even thinking of starting, or else you just go to wordpress and fit plugins together
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.
this is why we didn't teach framework to new coders. script kiddle wrote a dog water software and cost the company, thousands of dollars. most new coders blindly trust the framework without knowing what happen under the hood.
Reminds me when I was pen testing a wp plugin it says do not chmod 777 the backup dir when using php, of course the plug-in did and exposed the db to anyone with the dir name, ie elemantor/backup/db 😂
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
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.
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.
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).
And that's the purpose of proper Guard Clauses: if (empty($data['email']) || !filter_var($data['email'], FILTER_VALIDATE_EMAIL)) { echo '{"status":"500","message":"Invalid Arguments."}'; return false; } Real Code taken from my backend ... I just had to look when I saw the video.
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.
Always always always sanitize data coming from somewhere else, regardless of if the source is trusted
10:10 haven't you read the tweet from tib3rius? It is better to use AND 1=1 instead of OR 1=1
Yolo
what is the "root" cause 🥁badumm ts
I think the reason he tested for it is because he should've been the auth engineer of whatever framework people used lol
when a website uses php you can already assume you can take full control of the server somehow.
That's a bold claim and completely false. Get some experience as blue team under your belt and you'll realize it's just poor implementation.
@@TheParog unless you make everything yourself. perfect implementation I don't believe it's ever safe
@@NootNooot There's a huge difference between this statement and your last one. That's all I was pointing out.
@@TheParog but practically. Any php you see in the wild is wayyy from perfect. Php allows for sooo much error space. It isn’t even funny
@@NootNooot Any code in the wild is far from perfect. Starting at PHP 7.4, PHP can be a type-safe language. The rest of this video is about security by design fail. This is language-independent. You clearly don't use PHP in a stack that has secure information, so you're not up to date with the changes, but I am. You're over generalizing and making yourself look like you don't know what you're talking about to people with knowledge about PHP.
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.
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.
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!
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
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.
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
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.
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!
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.
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.
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.
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
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.
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!
Please do a video about the response manipulation topic you mentioned at 2:18
I mean if your Login Query is "PASSWORD && USERNAME" and if you don´t find a matching entry the password is wrong... Please stop developing Production software... This type of Query is only Possible if the Password ist stored in clear Text... You should never do this. The query should only search for the Username and than compare the Password Hash (like Bcrypt) with the send Password. So maybe this is not a well known issue, because if you have this issue you don´t care about Security in the first place...
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!
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.
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.
If you decide to write such horrible code and not validate user input then you are a bad programmer. It doesn't have to do with a specific framework, codeigniter should have validation like all frameworks, implement it. This is a bug that a junior PHP programmer working with the language more than 6 months should never make
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 ^^
Good thing my projects are so inconsistent I need to map the front-end keys to different variables anyway 😅
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.
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.
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.
Now thats some cool content
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.
It was awesome. When I saw $where = []; I gave the same reaction when you saw that 'root' came from the xml formatting bug
"What's the root cause" I see what you did there. You sly 4:42
HAHAH Our entire system is based on CI. Off to work I am!
When is next minecraft hacked?!?! I mis it.
This blog post and tweet sound AI-generated. 🤣
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)
Remmeber guys, don't trust user's input.
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.
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?
I saw that pen spinning pen at 4:02 👀
He is a "pen" tester..
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 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 ❤
11:20 I never facepalmed so hard in my life. This is clearly on Codeigniter. If you tell users that you sanitize input, you sanitize input. Technicalities about keys and values are useless. If they can't fix it, they should alter their doc to push sanitizing onto implementers.
Communication is very important.
I would argue that the first example isn't really relevant to the framework - any ORM library would happily allow returning expected rows if the programmer skips passing a "where" clause based on their incorrect logic for detecting parameters - if your implementation is true, it really feels like a programming error and not related to the framework. Plenty of other ORMs would allow the same thing:
```
# Mockup using python alchemy
query = User.query()
if request.json().get("username") and request.json().get("password"):
query = query.where(User.username==request.json().get("username"), password=request.json().get("password")
return query.first()
```
The second on the other, as opposed to some other comments, to me, definitely feels like a short-coming of the library. A query method that _should_ accept key-value pairs for fields and values should probably sanitised the field and the value - I'd imagine passing some custom sql, that's designed to append to the where clause, should be a dedicated method of the library (or maybe some attribute to denote a 'raw' value) to avoid this kind of mistake
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.
Server: "What is your username and password?"
Hacker: "Yes."
Server: "Welcome back, Sir."
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?
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.
stupid code, lazy devs and (ab)using public frameworks without knowledge.... yeah the perfect recipe for getting ya ass hacked.
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
Can you show the contents of the \Config\Services::request() method? I am having a tough time recreating your example
Honestly who writes code like that? This is embarrassingly bad. Just feeding user input into a framework like that. Or fucking up login like that.
Beginners surely can do that and it is ok but someone else should be looking over their shoulder and either the company or the 2nd pair of eyes should be ashamed.
Come on, falling for these just shows the limit of the sell made man in computer science.
Nowadays, especially if you plan to develop a custom website, these are absolutely mandatory to know about before even thinking of starting, or else you just go to wordpress and fit plugins together
Wait, isn't CodeIgniter doing prepared statements?
Or maybe that wouldn't help?
7:12 It may be obvious I don't use PHP, but that line 15... those backslashes. It just looks so WRONG
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.
this is why we didn't teach framework to new coders.
script kiddle wrote a dog water software and cost the company, thousands of dollars.
most new coders blindly trust the framework without knowing what happen under the hood.
What grinds my gears is that there is no frickin array anywhere in the json! It's an OBJECT omg...
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.
Reminds me when I was pen testing a wp plugin it says do not chmod 777 the backup dir when using php, of course the plug-in did and exposed the db to anyone with the dir name, ie elemantor/backup/db 😂
I usually return with 400 response when the username or password is missing from request body. Why go further
01:30 I still don't understand the "array" part. There is no "[" or "]" in JSON data. Where is array?
Why would you assume that the authentication check code has that mistake?
Isn’t that code that YOU wrote?
would be nice to pentest my highschool project php website (no frameworks used)
Hey Michael Cera, i didn't know you were a Sec Research guy, loved you on Year One!
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
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.
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.
I think this has nothing to do with codeigniter or even PHP, this is more a logical problem of how use the "if".
Wow LiveOverflow invented new header "Cintent-Type" (7:17)
Code ignitor offers validation, which would prevent this.
This is just incompetence and lack of testing. Lack of input sanitizing.
CodeIgniter should not be used - Laravel is a much better option.
Awesome video. Big thanks to Eslam. Great job 👏
can I ask about the sql payload what is the \" in the payload I don't get it
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.
have a like for the 'just the theory, a coding theory' =)
What exactly should i learn to understand this? Just point me in the way
That's why I just love Doctrine.
Klassiker. First user is usually the admin.
Hey, ich habe dir eine Email geschrieben, bzw. zwei. Jetzt weiß ich gar nicht ob die angekommen sind ^^
Rule no. 1:
Don't trust any input
And back in the early 2010s they were saying SQLi is dead
too many bad programmers in the world.
"array" _cries in dictionary._ both the data type and the book :P
I have seen this very same issue in another custom framework.
Dude how does these things even come to you lmao what a GOD !!
Key not escaped?! That sounds like a big issue
you look like non-swole version of Jeff Nippard
Game theory and bob ross references landed hahaha awesome
Great video
How it looks like => what it looks like
And that's the purpose of proper Guard Clauses:
if (empty($data['email']) ||
!filter_var($data['email'], FILTER_VALIDATE_EMAIL)) {
echo '{"status":"500","message":"Invalid Arguments."}';
return false;
}
Real Code taken from my backend ... I just had to look when I saw the video.
root for me is always a root element within web context
Also, root array makes no sense, rather root object