0

Showing user data on a page by query:

$query = "SELECT * FROM COLLECTIONS WHERE uid = $_GET['user_id']";

But problem is that user can see other users data by changing that uid. How to solve this problem.

Rajeev Malik
  • 9
  • 5
  • 1
    You need to implement access control, checking that the collection belongs to the logged in user. – Barmar Apr 10 '18 at 17:59
  • 4
    Other problem is.... **Your code is vulnerable to SQL injection and will be hacked** even if [you are escaping inputs!](https://stackoverflow.com/a/5741264/2595450) Use [Prepared Statements](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) instead. – Spoody Apr 10 '18 at 18:00
  • 3
    Don't get the userid from a URL parameter. When the user logs in, set a session variable, and use that. – Barmar Apr 10 '18 at 18:00
  • encrypt the user_id so the user will not change it. – abrar Apr 10 '18 at 18:01
  • Like this`$query = "SELECT * FROM COLLECTIONS WHERE uid = $_GET['$_SESSION['user_id']'];"` @barmar – Rajeev Malik Apr 10 '18 at 18:02
  • 1
    @abrar that's too much – Spoody Apr 10 '18 at 18:02
  • @RajeevMalik Basically just check if the user has the right to access the item from `collections` if so, show it – Spoody Apr 10 '18 at 18:02
  • @RajeevMalik `$_SESSION['uid'] = $uid;` – Barmar Apr 10 '18 at 18:02
  • How to do that @ mehdi – Rajeev Malik Apr 10 '18 at 18:04
  • General approach is to get the item and see if the user has the right to see it (does the user own it), I can't tell for sure as I don't know what data are you storing in that table nor it's structure – Spoody Apr 10 '18 at 18:05
  • what's the problem in using session variables, can u give me some example code of your approach @MehdiBounya – Rajeev Malik Apr 10 '18 at 18:07
  • There is no problem with the approach @Barmar suggested – Spoody Apr 10 '18 at 18:08
  • @RajeevMalik Is this a hobby website? I really hope you're making this just for fun, and nobody's storing their passwords or confidential data in this. – wizzwizz4 Apr 10 '18 at 18:39
  • @MehdiBounya - I would say encrypting it is not enough, they would need to add a different random salt to each user's id, to prevent me from breaking any simple hashing they may attempt to do, – ArtisticPhoenix Apr 10 '18 at 20:42
  • @ArtisticPhoenix not sure if you are serious xD if you are, just checking for permissions is enough. – Spoody Apr 10 '18 at 20:47
  • I am serious, you cant just hash the ID and think it's safe. I could brake it because I know it's a number, all I would do is say it's 32 long, it's md5, then i hash my ID if its the same as that hash, well I can take another id and just hash it. if it's not but has the same salt for everyone, then I can brute force that salt from my hash and user id, so it would have to be a random salt, which requires storing it, that was my point it's easier to do access control at that point. I was replying specifically to this comment `encrypt the user_id so the user will not change it` – ArtisticPhoenix Apr 10 '18 at 21:43
  • @ArtisticPhoenix My comment was that hashing and encrypting is too much and unnecessary (as you said _it's easier to do access control at that point_), another simpler approach is to generate a unique key that is long enough to prevent brute forcing and store it along the primary key (int id), and use the unique key for public accessing. – Spoody Apr 10 '18 at 21:54

1 Answers1

0

Take your website offline. NOW. Somebody is going to either wipe the data or steal the data or inject malware that's served to all of your customers

Breathe. You've bought yourself some time, assuming it hasn't already been breached.

A small subset of the security measures you NEED to take

These mitigate, in order of "has biggest immediate benefits" to "is probably most important", one problem each. (Apart from number 3, which mitigates anywhere from 4 to 32241 problems of equal or greater magnitude to number 1.)

  1. Look through every instance of every database request, and make sure that you are never using double quotes or the . operator when defining your query string. Rebuild all of your database handling code to use some sort of parametrised SQL query system.
  2. Use an authentication library, or at the very least a crypto library.
  3. Ask about your setup on Security Stack Exchange using an account that is in no way traceable to your website. Not even to your company, if your website is associated with your company.

Why?

Yes, I know, that website is probably important and needs to stay up so people can use it. But try this:

www.badwebsite.com/your/page/here?uid=1 OR 1

All of the data is visible! You are accepting code from the user and running it in your database. Now what if I decided to delete all of your database tables?

That's just covering the first point I made. Please trust that there are bigger problems for your users if you haven't done step 2, the least of which is hundreds of their accounts on other websites (e.g. Gmail, Example Bank) becoming known to cyber criminals.

Take a look at this comic strip:

The server crashes if the user's password is a resolvable URL.

There's also a unicode-handling bug in the URL request library, and we're storing the passwords unsalted ... so if we salt them with emoji, we can close three issues at once!

This is made to be more funny, but the problem described in this comic strip is probably less bad than the problem you are facing. Please, for the sake of whoever has entrusted you with their data, turn it off for a few days whilst you try to make it something resembling secure.

You might want to bring in a technical consultant; if your developers are not experienced in creating intrusion-proof software then they're probably not up to the task of making insecure software secure (which is orders of magnitude harder, especially if you're new to that sort of thing).

wizzwizz4
  • 6,140
  • 2
  • 26
  • 62
  • 1
    That injection `"1;CREATE TABLE you_have_lost (number int);"`won't work op PHP's MySQL clients because they dont support separated SQL statements separated with semicon (`;`)... Your just getting a error instead without executing the queries and without created table.... unless offcource you are using mysqli_multi_qeury() then your SQL injection would work.. – Raymond Nijland Apr 10 '18 at 18:18
  • @RaymondNijland Thanks. Can you think of a simple, easy to understand demonstration of an injection that _would_ work in this case? Some way of reading all of the data? (I don't think SQL's as bad as JavaScript in that regard.) – wizzwizz4 Apr 10 '18 at 18:20
  • @RaymondNijland That's the classic case; I don't know why I didn't think of it. Thanks! – wizzwizz4 Apr 10 '18 at 18:27
  • 1
    The php page could just show one record in that case add `LIMIT 1`, `LIMIT 1, 1`, `LIMIT 2, 1` end so on to the SQL injection see http://sqlfiddle.com/#!9/b3f901/9 – Raymond Nijland Apr 10 '18 at 18:32
  • @RaymondNijland Why don't you make the edit, so you're credited in the edit history? – wizzwizz4 Apr 10 '18 at 18:34
  • I would do `1 OR 1; -- ` comment the rest of the SQL, then no `LIMIT` Here is you limit fiddle I just hacked it http://sqlfiddle.com/#!9/b3f901/11 – ArtisticPhoenix Apr 10 '18 at 21:48