PHP Data Grab Project - Need help with security

[ClipboardCode]

PHP Data Grab is a project I just started recently for an easy way to configure connections and queries but to make them more dynamics especially when dealing with multiple web servers/database servers (training, qa, dev, prod,...). I currently developed internal only and I can say my security skills are a little lacking. I do plan on implementing prepared statements as a new dynamic method and add a dew page security methods.

 

I would like to get some help as I really want to contribute to the PHP community but also have a safe product (it is free). 

 

I came to this site hoping to get feedback and did not know the project was as bad at first glance as some have made it out to be. An eye opener for sure.

 

I went ahead and put a BETA - TESTING - WARNING message on the download link until I can shape it into a secure project.

 

EDIT : Product plugs removed

Edited February 12, 2017 by Barand
Advertising removed

[benanamen]

To start with, warning message or not, that code should not be so freely available as it is. It will get downloaded and find it's way around the Internet. Take it down and post relevant code in the forum for help. I can't stress it enough, the code is EXTREMELY DANGEROUS.

 

The very first thing you need to fix is the if plaintext == plaintext log me in problem. While you're in the code, get rid of all the @error suppressors. Errors are your friend. They tell you something is wrong and needs to be fixed. Get rid of the variables for nothing, Put the Javascript and CSS in a separate files. Your app relies quite a bit on Javascript to work. If JS is turned off, the app is useless.

 

Once you have a secure solution to the login we can go from there. No point getting into anything else until you do that. I will leave it to you to see what you come up with. Using SQlite might be a good option for you.

 

FYI: Your server is vulnerable to a Clickjacking Attack and you are advertising to the world what server and version your running.

Edited February 12, 2017 by benanamen

[ClipboardCode]

To start with, warning message or not, that code should not be so freely available as it is. It will get downloaded and find it's way around the Internet. Take it down and post relevant code in the forum for help. I can't stress it enough, the code is EXTREMELY DANGEROUS.

 

The very first thing you need to fix is the plaintext if plaintext == plaintext let me in problem. While you're in the code, get rid of all the @error suppressors. Errors are your friend. They tell you something is wrong and needs to be fixed. Once you have a secure solution to the login we can go from there. No point getting into anything else until you do that. I will leave it to you to see what you come up with.

 

Thank you for the help. Few questions on the login security. The password does get sent as POST and with the @ it will just cause an error if the POST variable is not there so I use @ just to prevent an error I do not care about trapping. If it is there then I use it. I do need help figuring out better security but I was under the impression that PHP code is secure as meaning you have to have access to the PHP code file itself in order for it not to be secure. meaning I would not store the password in a client side variable but the following line:

 

if (@$_POST['letmein'] != $dg_SecurityWord){}

 

is all PHP. Any direction would help.

[benanamen]

the @ it will just cause an error if the POST variable is not there so I use @ just to prevent an error

 

I am a bit confused. Did you write this app? If so, you can do all you did but you don't know a basic thing as how to check if there is a variable or not?

 

As far as the login security, your starting point is learning how to use  password_hash and password_verify

Edited February 12, 2017 by benanamen

[ClipboardCode]

I am a bit confused. Did you write this app? If so, you can do all you did but you don't know a basic thing as how to check if there is a variable or not?

 

Yes but since on this particular variable I do not care about trapping the error the @ works perfectly fine in this situation. The first time you come into the page the variable is not present and therefore I already had it's value as an empty string. The @ is there just to prevent an error message from showing on the screen. I will add extra code just to test for the variable even though the functionality of the code does not change. I guess for simple things I sometimes just use a shortcut method and not the official proper way even though both methods do work. I will add this to the change list. 

Edited February 12, 2017 by ClipboardCode

[benanamen]

If you want to be a good coder, get rid of the "works perfectly fine" mentality and adopt a "Best Practices" mentality. Lots of things "work perfectly fine" but you should never do it for that reason.

 

We are not talking about "trapping" anything. The proper form procedure is basically

if ($_SERVER["REQUEST_METHOD"] == "POST") {
// Check if expected variable has a value, then do something
}

[Jacques1]

I appreciate that you're willing to take suggestions, but let's be realistic: This is not about adding a bit of security here and there (whatever that means). It's about (re)learning fundamental aspects of programming, because the whole code is written in a very naive manner. Which is somewhat odd after 25 years of experience.

 

I also don't see how this is useful. It's a big GUI which wants my credentials and write access to my server, and essentially all I get is a broken reinvention of prepared statements. Why not use real prepared statements then?

 

Personally, I would write this off as an exercise. Learn PHP, do your own private projects and then publish code. This is how we've all started.

[ClipboardCode]

I appreciate that you're willing to take suggestions, but let's be realistic: This is not about adding a bit of security here and there (whatever that means). It's about (re)learning fundamental aspects of programming, because the whole code is written in a very naive manner. Which is somewhat odd after 25 years of experience.

 

I also don't see how this is useful. It's a big GUI which wants my credentials and write access to my server, and essentially all I get is a broken reinvention of prepared statements. Why not use real prepared statements then?

 

Personally, I would write this off as an exercise. Learn PHP, do your own private projects and then publish code. This is how we've all started.

 

I m completely self taught which may be a big part of my problem. I also currently develop for internet (intranet/network) app that do not require much or any security as the information is not sensitive. The project here I have used for years and it has worked for me so I thought about sharing it. This project may not be for everyone as the GUI part was the main aspect so others would not have to write any code other that function calls. In my mind I though the project was ready to share but my eyes are open now from the comments on this forum. I want to do the right thing and I love learning code. My learning curve has always been what is the problem, ok lets code a solution. I learn as I go and I understand there are 50 ways to do the exact same task and for me have felt as long as it is not overly complicated or round about I will do any method that works. Some methods I may find via a book, google search, or just testing. Some methods I do not even know about and apparently from the remarks there are plenty.

 

I will not be promoting this tool anymore on the site on other topics but I still would like to help where I can for small bits of code. I also am staying positive and will be using this as a learning curve and would appreciate any help at all. This is what I thought this site was for and understand the security concerns. I have placed this project in BETA and including a ton of warning messages. I will even be re-writing parts of the GUI this afternoon to clearly state everywhere while in BETA this should not be used on any real website. This live project is just a few days and other that the YouTube video I made for use on the web site this forum is the first time I have promoted it at all.

[ginerjm]

And don't use the @ operator. Why in h... would you?

[ClipboardCode]

And don't use the @ operator. Why in h... would you?

 

As I said earlier it was used as a shortcut since in my situation error handling was not important for such a variable.

 

For example I wrote it like this:

 

The below will not error or write the string if the GET variable is not found. Also it will not write the string even if the GET is present but empty (Example = mysite.com/myfile.php?test=).

if (@$_GET['test'])){
  echo "Found it";
}

Now if I wrote it like this then it also would not error or write the string if not present but it would still write the string if present but empty (Example = mysite.com/myfile.php?test=).

if (isset($_GET['test'])){
  echo "Found it";
}

So in order to not error or write the string when either not present or present and empty I would have to write it like this.

if (isset($_GET['test'])){
  if ($_GET['test'] != ''){
    echo "Found it";
  }
}

So I wrote it with an @ since it works perfectly find and was shorter at the time. We can agree yes there are proper ways to do things but they both still work and since it was not a crucial part of the security or functionality I am not sure why we are latching on to this minor thing so much as there are bigger parts of the program I have questions on.

 

I will be making changes and want to address some issues but so far it has been 100% negative comments and I do get where many of them are coming from. I am not sure if anyone sees where I am going with this project. This project is diffidently not for many readers of this forum as you can all probably cod anything you want so why would you need to use someone elses code? In my job we have very few developers but still write some major applications. I have a staff that do not know how to program but can still handle some basic queries. With a tool like this I can have them add, edit, and manage connections and queries for multiple web server paths without the need to go into code.

 

Now getting back on topic about the password login. I am still not sure in the direction to go as the password check is done in the PHP code and never the client side code. Am I misunderstanding that having the password on only the PHP side is not secure enough? I was under the impression that only if you had access to the PHP file meaning you could see the PHP side code only then was the password unsecure. My login simple refreshes the page with the user enter client side password back to the PHP files and then the PHP determines if it is correct and only then includes the admin file. The admin file itself has another security check where if a PHP variable is not detected that is availble only on the login php file then it will not load the page at all and die. The only unsecure part I can tell is there is a default password with the package but in the instructions I state this and show how to easily change it but there again you have to have edit access to the PHP file. So again help me understand what methods I am missing if doing everything PHP code side is not secure?

Edited February 13, 2017 by ClipboardCode

[requinix]

And don't use the @ operator. Why in h... would you?

Aside from the few remaining functions in PHP which issue warnings in cases that don't need them, @ is fine if you are already accounting for potential problems being covered up; note that custom error handlers will still receive the warning, but with $errno=0.

@$_POST["letmein"] != $foo

!isset($_POST["letmein"]) || $_POST["letmein"] != $foo

accomplishes the same thing either way, but using @ lets you avoid the tedious isset() check. With PHP 7 you could swap the @ for a ??

($_POST["letmein"] ?? "") != $foo

[Jacques1]

You are in fact putting the plaintext password into the client-side code:

params['letmein'] = '<?php echo $dg_SecurityWord; ?>';

So anybody who manages to steal the admin session gets the password for free.

 

But even without this trivial vulnerability, there are countless of ways how the password might be obtained:

• Through an SQL injection vulnerability; MySQL can import the content of files
• Through another script which accepts arbitrary file references and loads those files
• Through a misconfiguration; the only thing which makes the webserver execute .php files rather than serve them as plaintext is a single parameter, and this can break
• Through a timing attack; a simple character-by-character string comparison leaks information about the reference string through subtle time differences: The longer the common prefix of the two strings, the longer the comparison takes
• …

But as it turns out, the password is not even necessary. Anybody can POST data to your save script, and you'll happily write this data to the configuration script. It's even possible to inject arbitrary PHP code.

 

I repeat: Anybody can upload and execute arbitrary PHP code on your server. This is the worse case scenario.

 

Trying to fix this will only reveal more vulnerabilities and defects. The core problem is that you really don't know how to program. Sure, you can produce code which seemingly “works”, but you don't understand fundamental concepts like validation, robustness, defense in depth. You've been programming in a strange bubble where appearently neither you nor anybody else cared about the quality of your work.

 

I realize this is brutal feedback. But you have to understand that you're essentially a novice programmer starting from zero. You haven't learned much except for a few bits and pieces of syntax.

[ClipboardCode]

You are in fact putting the plaintext password into the client-side code:

params['letmein'] = '<?php echo $dg_SecurityWord; ?>';

Yes but this is only after  a successful login on the admin page. If you are on the admin page then you already know the password and I brought the password back to the admin page only for refresh reasons to not require you to login again. The save script requires the correct password though so not sure how someone could use that without it.

 

Yes I do agree I have been coding in a bubble but I have accomplished many projects and have saved companies millions of dollars. So far I have not been giving a task I have not been able to code and for me getting the job done was the goal. My entire philosophy of coding has always been to keep it as basic as possible so even a novice programmer can come after me and take over. Luckily they have all been internal apps and can agree I have lacking security skills. I do rely to much on PHP itself as being my security. 

 

 

[ClipboardCode]

Here I created a version for only discussing the password security.

 

1. User logins with password.

2. Temp form is created and the page is refreshed with the password being sent in the parameter "letmein" using POST.

3. The PHP checks "letmein" against the PHP variable $dg_SecurityWord and if not correct the user is present with the login again. If correct move to step #4.

4. If password correct the page "secure_page.php" is included.

5. The file "secure_page.php" has a PHP variable check at the very top looking for the PHP variable $dg_SecurityWord. If not found the page dies. If found the page loads.

 

No where in this process is the correct password exposed to the client. Now in the original code (not show in this example) the password is revealed to the client code for refresh without login reasons. This only occurs after a valid login though.

 

INDEX.PHP

<script type='text/javascript'>
function openWindowWithPost(url, params, newWin){
	//Creates a temp form in order to use POST instead of GET to pass paramaters to another site.
	//Done this way so a form does not need to be created manually.
	var form = document.createElement('form');
	form.setAttribute('method', "post");
	form.setAttribute('action', url);
	form.setAttribute('target', '_self');
	for (var i in params){
		if (params.hasOwnProperty(i)){
			var input = document.createElement('input');
			input.type = 'hidden';
			input.name = i;
			input.value = params[i];
			form.appendChild(input);
			if (newWin != undefined){
				form.target = '_blank';
			}
		}
	}
	document.body.appendChild(form);
	form.submit();
}

function login(){
	//Calls this exact same page passing the password in the paramter 'letmein' using POST.
	var params = {};
	params['letmein'] = document.getElementById('pswd').value;
	openWindowWithPost('index.php', params);
}
</script>

<?php
$dg_SecurityWord = 'password'; //The password but only stored in PHP unless you make it to the secure_page.php with a correct password.

if (@$_POST['letmein'] != $dg_SecurityWord){ //Check to see if valid login
	//No valid login display the login prompt
	echo "<input type='password' id='pswd' /><button onclick='login();'>LOGIN</button>".PHP_EOL;
} else {
	//Valid login show the secure page.
	include('secure_page.php');
}
?>

SECURE_PAGE.PHP

<?php
/* 
If the PHP variable $dg_SecurityWord is not found the page will not load past the first line of code.
This way you cannot directly go to this page without going through another PHP file that has this
variable setup and uses this page as an include/require.
*/

if (@!$dg_SecurityWord){die('This is a secure site. Please use the login file (index.php).');}

echo "YOU MADE IT TO THE SECURE SITE";
?>

Created test here http://clipboardcode.com/security_check/index.php

Edited February 13, 2017 by ClipboardCode

[Jacques1]

You don't get it.

 

I've just told you that your application allows anybody to upload and execute malicious PHP code, and you're worried about passwords. But even then you don't even try to implement what we've told you. Have you never heard of hashing? Sessions? Anything?

 

If you are on the admin page then you already know the password [...]

 

No, you do not. Since your application has XSS vulnerabilities all over the place, anybody can get the password simply by injecting a script which reads that password:

 

 

 

 

The save script requires the correct password [...]

 

No, it does not. If you think I'm stupid, read the code. It takes the input and writes it to the settings script. It doesn't even mention the “letmein” parameter.

 

 

 

[...] getting the job done was the goal [...] Luckily they have all been internal apps [...]

 

I'm really starting to think that you're a hopeless case.

 

A program which blows up when somebody enters the name “O'Neill” and which gives any random user direct access to the underlying system is not “getting the job done”. It's fundamentally broken. It violates even the most basic requirements. It's effectively malware.

 

Forget about learning PHP. You need to understand what software even is, what it's supposed to do, what it mustn't do.

Edited February 13, 2017 by Jacques1

[ClipboardCode]

You don't get it.

 

I've just told you that your application allows anybody to upload and execute malicious PHP code, and you're worried about passwords. But even then you don't even try to implement what we've told you. Have you never heard of hashing? Sessions? Anything?

 

 

No, you do not. Since your application has XSS vulnerabilities all over the place, anybody can get the password simply by injection a script which reads that password:

 

screen-1.png

screen-2.png

 

 

 

 

No, it does not. If you think I'm stupid, read the code. It takes the input and writes it to the sessions script. It doesn't even mention the “letmein” parameter.

 

 

As I already said I know it writes to the session script but only after validating it was correct and NEVER before. The part you are looking at for the client side password view is after validation for refresh reasons only. Look at the password example I gave and show me how that is unsecure.

 

As for your screens shots all that is from the admin tool after validation. If you want to injects scripts or do anything then that is on you as you got to the admin tool by entering a correct password. If you had access to the admin panel then you are considered a secure user.

 

I am concentrating on the login problem since it was on the first post reply that this needs to be fixed first but you still cannot anywhere my question is to how the login is unsecure. If it takes a valid password before I ever write that part to the client how is having the password in client side any help to a hacker that aparently somehow already had it? Look at just the password example and please tell me how that is unsecure. If it is then what part is unsecure? The PHP code is what is doing the password varification and I did read the code here is the line "if (@$_POST['letmein'] != $dg_SecurityWord){". This is PHP code not JavaScript. See example above.

 

1. User logins with password.

2. Temp form is created and the page is refreshed with the password being sent in the parameter "letmein" using POST.

3. The PHP checks "letmein" against the PHP variable $dg_SecurityWord and if not correct the user is present with the login again. If correct move to step #4.

4. If password correct the page "secure_page.php" is included.

5. The file "secure_page.php" has a PHP variable check at the very top looking for the PHP variable $dg_SecurityWord. If not found the page dies. If found the page loads.

 

I am truly trying to understand but your method of going off on me is not helping at all. Maybe chill a little bit and try understanding my basic question one at a time. I will restate it again: How is the password login unscure as it never writes the password until after it has validated the password using only PHP code PHP POST and PHP variable)?

Edited February 13, 2017 by ClipboardCode

[benanamen]

This is going to be a reeeelly long thread. OP, the problem is you don't know what you don't know. You know enough about using a saw to cut your fingers off and that makes you dangerous in the coding world. You are not going to have any luck trying to argue what you think you know with people that actually do know what they are talking about.

 

With a tool like this I can have them add, edit, and manage connections and queries for multiple web server paths without the need to go into code.

 

If you just needed to get the job done, you could have saved the company and yourself even more time and money if you just used the free Mysql Workbench or Phpmyadmin. Aside from that, there are numerous other tools that do the same thing, free and paid. You are trying to re-invent the wheel instead of getting the job done.

 

If you really want to be a programer, listen carefully to what we tell you and apply it and get used to harsh criticism. There is a whole lot you don't know.

Edited February 13, 2017 by benanamen

[ClipboardCode]

This is going to be a reeeelly long thread. OP, the problem is you don't know what you don't know. You know enough about using a saw to cut your fingers off and that makes you dangerous in the coding world. You are not going to have any luck trying to argue what you think you know with people that actually do know what they are talking about.

 

 

If you just needed to get the job done, you could have saved the company and yourself even more time and money if you just used the free Mysql Workbench or Phpmyadmin. Aside from that, there are numerous other tools that do the same thing, free and paid. You are trying to re-invent the wheel instead of getting the job done.

 

If you really want to be a programer, listen carefully to what we tell you and apply it and get used to harsh criticism. There is a whole lot you don't know.

 

Since no one can answer one question I have instead of trying to look at the entire functional program I think I will need to go to a different forum and ask just 1 question at a time in order to stay on target. I make a great living as a programmer that sucks. I understand I do not know it all hence why I came to a PHP help forum asking questions. 

[Jacques1]

Your question has been answered at least three times now. You've chosen to ignore the replies, so it's pointless to continue.

 

I think I will need to go to a different forum

 

I won't stop you.

 

 

 

I make a great living as a programmer that sucks.

 

No doubt about that. Like I said, you're living in a bubble.

Edited February 13, 2017 by Jacques1

[gizmola]

Hi ClipboardCode,

First off I understand that your feelings have gotten hurt. You came here with an app you are proud of, and want to share with people, and you thought was already pretty good, and instead you got crapped on, or at least that is your perception.

I hope you can at least try and understand that the people responding to you are hard core best practice professional php/web developers with years of experience, and a highly opinionated stance on best practices and a low tolerance for security issues.

It also seems that you didn't get to some of the questions you felt you had, which is a shame.

But the obvious things to me, that would ultimately be of value to you is in the developing of an understanding of the state of the art in terms of professional php development, as well as the modern state of open sourcing and sharing a project with the world.

That would include things like:

• Use of Git/Github or Bitbucket for your project code, since you intend to share it freely. Obviously github is ideal for this.
• Use of php component libraries where appropriate
• Good clean code with MVC, separation of logic & presentation
• Best practices for security, and a security audit
• Open source licensing

Let's just say your code gets out into the world, and it has huge security holes, which clearly it does. Would you want your reputation and the reputation of your company to be attached to that from here on out? While I come to this late, and have never seen the code in question, the people who did, and who replied to you, have expressed that concern.

It seems like what people have been saying to you is that you could use a ground up rewrite, and that is certainly a bitter pill to swallow, but that effort could be enormously valuable to you as a learning experience. It might not have been what you originally were looking for, but the potential for that still exists for you, if you so desire.

In the meantime, I highly recommend that you familiarize yourself with the symfony components, use of composer, and perhaps even read a little about dependency injection. I have recommended this video to many people as a way of starting to understand how modern php development had evolved into something competitive with other language ecosystems and the reasons that needed to happen before php became obsolete. This is almost 5 years old now, so lots of improvements in php and the component ecosystem, but it's still a great introduction: "Wonderful world of Symfony components"

A php centric introduction to Dependency Injection: http://fabien.potencier.org/what-is-dependency-injection.html