# Tag Info

33

SQL Injection alarm Don't use user passed text in your sql statements if you aren't using parameters. From: https://stackoverflow.com/a/7505842/2655508 Using parameters helps prevent SQL Injection attacks when the database is used in conjunction with a program interface such as a desktop program or web site. In your example, a user can directly ...

18

Cleartext passwords Not a code-specific issue exactly but it would appear that you are storing the user's passwords in clear text. This means that anyone gaining access to the LOGIN_Tab would be handed a list of valid usernames and passwords for your application. This is especially bad when combined with SQL injection vulnerabilities! Consider instead ...

17

This is one of those places where if you ask 10 developers the same question, you will get at least 15 answers, and they are all valid. My 2 cents: It's 2014. Use HTML5. Discover jsfiddle.net for this kind of thing. Put "required" on the username and password fields, it's free validation. There's nothing wrong with in-page CSS. I often mix sitewide and in-...

16

In addition to Heslacher's answer I would also look at the using statement. It is very important to keep a handle on resources like Sql Connections to ensure they are closed and cleaned up. Link to the MSDN about SqlConnection with an example with the using statement. A SqlConnection object represents a unique session to a SQL Server data source. ...

15

This is not secure at all. Apart from the GET issue (use POST) and the hashing issue (use bcrypt) already mentioned, and the fact that == is not timing safe (use hash_equals), an attacker can log in without knowing the password like this: login.php?userpass[]=1 The reason for this is that strcmp("STRING", [ARRAY]) returns NULL, and NULL == 0 is true. You ...

14

Try using std::vector instead of arrays. As a result, you'll not be required to keep track of the array_size in a separate variable like n in your case. Something like this: std::vector<std::string> username; username.push_back("admin"); //other entries go here std::vector<std::string> password; password.push_back("root"); //other entries go ...

14

Yes, there are several things I can see with your implementation, from most critical to less critical: SQL Injection - You has it! Since you're already using mysqli, learn about prepared statements with mysqli and apply them. You are very vulnerable to Little Bobby Tables. Your password hashing is not secure - sha256 is insecure for password hashing in on ...

13

Preamble As a matter of security, I can safely say that you would fail a professional security audit in seconds. You should be including the hashing library as an external component. You also need to forget everything you know about querying databases and start again. Also, look at Model-View-Controller. The fact you have a templating engine and separate ...

13

Security-wise, you need to make sure you a) never trust the user and b) never trust the user. You should realise that anyone has access to the GET parameters, lines like <?php echo @$_GET['msg'];?> should thus be avoided. Always call htmlspecialchars() on stuff you get from users etc when printing output. Otherwise I could craft an URL like http://... 13 Here are some observations: you first iterate all rows in Credentials and then select the Password yet again - why not just select the pasword? seems like your password is plain-text - BAD idea - hash your password together with a salt please don't concat strings to get SELECT queries - use parameters instead you are using C#/ASP.NET so why don't you use ... 12 You have some compiler warnings:$ clang++ -o console{,.cpp} console.cpp:24:1: warning: control may reach end of non-void function [-Wreturn-type] } ^ console.cpp:41:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ 2 warnings generated. You have a segfault: Enter username: a Enter password; b Segmentation fault: 11 For entering a ...

12

Never write login code This is much harder than you think. Leave it to the experts you will get some security piece wrong. Code Review Don't use this: using namespace std; See Why is “using namespace std;” considered bad practice?. The reason the "standard namespace" is called std is so that it is not a big deal to type std:: before each standard ...

12

Style Python has a style guide. It's called PEP8, and normally if you follow it you get easier to read code. It's a fairly small read, and you can learn some new things from it. It's recommended in PEP8 to limit the character width of scripts to 79 characters. It has a few benefits: People on StackExchange don't need to use the horizontal scroll. It's ...

12

Security issues are covered, so just a bit on the code itself. You have three nearly identical pieces of code, inside a switch-statement. That seems to be a code-smell. Apart from that, I'm missing a default case. What happens if a user logs in before you have assigned a role? He gets an empty screen, or an ugly error message? How about creating a(n ...

11

Disclaimer: I am no security guru. So a lot of what I say in regards to security are assumptions. The code review is good, but you should get a second, third, maybe even fourth opinion on my suggestions regarding security. I will denote each "opinion" that way you know to take it with a grain of salt. That being said, on to the review! Session Variables ...

11

Is my code properly commented? No. You're documenting methods where they are used (usually multiple places) as opposed to where they are declared (inherently only one place): /** * @param associative array * stripAllWhiteSpaces will remove ALL white spaces. * example: $stringBefore = ' this is an example'; *$stringAfter = 'thisisanexample'; */ $... 11 I'm no expert on security, but I believe this is not current best practice. I'll copy from an another answer of mine. Cryptographic Right Answers (2009, Colin Percival, author of scrypt) Password handling: As soon as you receive a password, hash it using scrypt or PBKDF2 and erase the plaintext password from memory. Do NOT store users' passwords.... 11 A few points: You can simplify the if loop by using the else condition. Getting input should be at the beginning of the while loop, because it makes more logical sense to put it there. It's what happens every time you restart. You can use the simple condition of True for your while loop. You don't need the continue keyword, as it is at the end of your loop. ... 10 These are all being used as global variables: string usrnam[] = { "admin", "user", "" }; string pasword[] = { "root", "default", ""}; string temp_pass; string temp_usr; bool loggedin; int n = sizeof(usrnam) / sizeof(string); This is bad practice because any of these variables can be modified anywhere at any time, which is a source of bugs and ... 10 You can replace all of the if(x.match(...)) return true; else return false; with: return x.match(...) !== null; or if you're content with returning a truthy/falsy value instead of proper booleans you can even simplify it to: return x.match(...) IsEnoughLength is ungrammatical. I'd use IsLongEnough You're mixing the password strength ... 10 I don't understand what you mean by "thrice nested", or why you call this an OS. I'm reviewing it as a login prompt. The entire rest of my code isn't bug free but this section is Beware of believing something is bug-free. It never is. This is especially important for code which needs to be secure, as this presumably does. Now for the review: Many ... 10 Right now, you're using the same salt for every user. This pretty much defeats the purpose of using salt in the first place. The point of using salt is to ensure that even if two users choose the same password, the hashed value will still be unique for each. So, to be useful, you compute the salt as a random string for each user, then store that random ... 10 Security Focusing on only the password hashing for a second, let's talk security. public function hash($string) { $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";$sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+"; $sting = md5(md5($fSalt) . md5($string) . md5($sSalt)); for($i=0;$i < 3; $i++) {$string = md5($string);$...

10

Security I'm not all that comfortable with echoing unsanitized user input: echo '<form action=' . \$_SERVER['REQUEST_URI'] . ' method = "post">';. I did not find any way to exploit it though. I asked a question on security.SE, maybe they can find a way. I would probably use SCRIPT_NAME instead. you should regenerate the session id via ...

10

Reinventing the wheel Please note that encryption should, in principle, always be two way. A 'one way encryption' just sounds weird. Normally something is encrypted with the intention to decrypt it at a later stage. This is not what you do. What you do is called hashing or perhaps 'encoding' as you indicated in your title. You could improve the formatting ...

10

It looks like passwords are stored in plain text form in the pswd.txt file. Never ever store passwords in plain text form. Store passwords salted and cryptographically hashed. That way, if an attacker gains access to the file, he still has to crack the password, which can be extremely difficult if the password is strong enough. To verify a password, apply ...

10

target = ARGV[0].to_s user = ARGV[1].to_s wordlist_file=ARGV[2].to_s Aren't those Strings already? network = 0 (1..5).each do if icmp.ping network += 1 end end would be more idiomatic as: network = 5.times.count do icmp.ping end #count will count how many times block returned true, exactly what you want. Ruby has "perlish" globals ...

10

Security There are quite a lot of beginners mistakes in your code. While there aren't any serious security issues right now, it does contain a whole bunch of bad practices which will sooner or later lead to security issues. If you follow some tutorial which suggested these approaches you used, I would suggest to use a more up-to-date tutorial which places ...

10

using as already covered SQL injection as already covered For getting a single row Reader is more efficient than DataTable Since user Name should be unique you should never have more than 1 row. And I think the syntax is cleaner. You don't need the userNameTextobx.Text == "" if (userNameTextobx.Text == "" || userNameTextobx.Text.Length < 4) You ...

10

Ok, moving down the list... I would separate the content of the includes folder into 2 semantically named folders: includes and classes - or if you want to eventually turn this into an MVC, maybe views and classes would be more appropriate. It's not uncommon to put database credentials in constants, but truth is you should never have more than one ...

Only top voted, non community-wiki answers of a minimum length are eligible