- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Two fatals errors combined result in The Real TRWTF.
Admin
So the web page looks something like this:
Enter password, or 'free entry': ___________
What's wrong with that?
Admin
The Real WTF is that they have both IsActive AND IsDeleted.
CAPTURE=And Again The Tumbleweed Blows Across Your Desk
Admin
Indeed what's a huge security hole compared to one BIT of redundancy.
Admin
I dunno. It might be possible to have an account that is not active nor deleted. I have used something similar, but the IsDeleted was really a MarkedForDeletion flag. An account that was marked for deletion could not be re-activated.
Personally, I prefer a single status field.
Admin
Not if there were four possible states for the user.
Captcha: I just used the generic one
Admin
That's a very (ahem) novel solution
Rather create the record initially with the invitation password.
I wonder if this was introduced in the code, or whether the initial spec said something like "The user should be allowed to log in with their invitation password at any time, in case they have forgotten their password"
Either way, the mind boggles...
Also, unions degrade performance. They should rather have added the invitation password check to the top query instead of redoing the whole query. Then of course the WTF would have been harder to spot ;-)
Admin
The first would be updated automatically by the software, and the second manually by an administrator. Keeping the fields separate makes the logic of updates simpler, at the cost of an extra line in the WHERE clause.
PS. The "preview pane" has a bit of a problem with Unicode. The by-line for this post now says:
"sometime in the future • by ĽudovÃÂÂÂt Å túr"
... and it keeps getting longer.
Admin
Storing plain text passwords in the user database is not such a bright idea either, though of course this is a relatively minor issue (IOW relative to the sheer insanity of inviting everyone to use everyone else's account). Even if you want to support password reminders, it's better to store the passwords in a separate table that can only be queried by the password reminder service.
And there are more secure solutions than password reminders, anyway. For example:
While this allows an attacker to hijack an account by intercepting e-mail, the same goes for ordinary password reminders, of course. But at least the attacker will not obtain your password, which happens to be the same combination you have on your luggage, and steal all your pants and canned oxygen. Furthermore, because the attacker has to change the password to log in (and changing it to the temporary password will be prevented), the user will notice that something is wrong the next time he tries to log in.
Admin
Admin
AND the password should be encoded in some way (although I personally do that business-side but what do I know).
Admin
IsActive could refer to: . email activation of an account . an unused account that has had data deleted but still exists (like yahoo mail if you don't log in for a while) . an account that is not longer accessible but has data attached to it that other users might see (eg forums) . etc, etc, etc
Admin
Interesting. It seems to be re-encoding each time. Unfortunately, the way you copied it already means it's blown up, so the closest I can get to your name from that (by decoding three times) is:
"Ľudovít ▒\xC2\xC2\xC2 túr"
I'm guessing that that's meant to be an Š in the middle there, but, yeah.
Admin
... or perhaps when some hacker has tried to brute force your password, and the account gets locked out: isActive=false and isDeleted=false
If you could delete an account by typing the wrong password a few times, that would be a wtf in itself.
Admin
Admin
Admin
Maybe I am missing something here, but...
If once the user changes their password, the orignal records Is_Active flag is set to 0, then the query will not return that record anymore. Yes, the union was a bad decision, but depending on how the logic works, I don't see how both records would be returned, if the flags get changed on the 1st record once the 2nd record is created.
Admin
OMG...why would you waste all that space of an extra flag?
If you want to store something to show if the account is currently active AND whether its deleted...you just need to use a single isDeleted String!!!
if isDeleted = "Yes" then the user is not active. else if isDeleted = "Pink" then they're not.
To indicate the deleted state, well heck you never heard of DELETE FROM...? we can just remove them from the database. If its there its not deleted...Come on thats obvious...
What? Oh well yeah i remember that requirement that you need to be able to restore a deleted user if they were deleted by accident....Yes ofcourse my solution fullfills it...after all thats what database backups are for... and we definately do one of those....were very particular...each morning i check that the back up tape has been ejected...ie backup succesfull and then i push it back in to make sure its all ready for the next one.....
... Jeeze you lot like to make things complicated.
Admin
What the hell are you talking about? Yup you're missing something.
Admin
(Hope this comes out all right - I'm not going to preview it...)
Admin
Note how the recordset returns EMAIL and PSWD, even though those were explicitly included in the WHERE clause:
SELECT USER_ID, FIRST_NAME + ' ' + LAST_NAME AS FULL_NAME, EMAIL, PSWD FROM USER WHERE (IS_ACTIVE = 1) AND (IS_DELETED = 0) AND (EMAIL = @EMAIL) AND (PSWD = @PSWD)I'm guilty of this... I mess around to get the query just right, and then forget to clean out unneeded return values. Not a biggie, but why send the same data both ways?
Of course, it could be worse... I still find code all the time with "Select *", and have to dig through the code to find out what fields are actually being used.
Or even worse, they could have a check like:
If rs("EMAIL") <> sEMail Then MsgBox "OH NOES TEH EMAIL IZ BORKD!"Admin
Did nobody else notice the unnecessary union on the same table?!
Ignoring all the other wtfs in this post, couldn't the query have just been:
SELECT USER_ID, FIRST_NAME + ' ' + LAST_NAME AS FULL_NAME, EMAIL, PSWD FROM USER WHERE (IS_ACTIVE = 1) AND (IS_DELETED = 0) AND (EMAIL = @EMAIL) AND ((PSWD = @PSWD) OR (DBO.GET_INVITATION_PWD() = @PSWD))
I know I'm not missing anything. ;)
Admin
just like Schrödinger's cat before you don't know if the user is existing or not :)
Admin
Keeping deleted acounts is a good idea, that way spammers cannot re-sign up with the same old email address. Of course some spammers are slightly smarter than this programmer and will plenty of throw away email accounts to work with. (A good programer could corrolate information from the deleted acounts to trap spammers, but this is beyond the idiot who uses one initial password for all accounts)
Admin
so TRWTF is this site ???
Admin
if they have already forseen hacker attacks the first thing to do is certainly not the introduction of an IsDeleted flag and then leave the door wide open for everbody to serve themselves. Hopefully this is not a financial institution
Admin
BTW, why not show a textbox on the screen with password in it and a text that says:
if this is your password and username click on enter otherwise logon with any username and global password
Admin
That actually is a possibility of what might be going on.
When they first send out an invitation, they store the email address on the USER table (is_active = 1, is_deleted = 0) then when/if the person registers they add a new record to the USER table (is_active =1, is_deleted=0) and update the old record to is_active = 0.
It's a crappy design, but accomplishes authenticating new users and registered users.
Admin
I think Ben's comment says it all: DBO.GET_INVITATION_PWD() returns the same password for all invitations.
Admin
We used to put in a master backdoor password that could be used to login as any user. Very convenient when you had to support users and had to check what they were seeing :-)
Admin
Both parts of the UNION query are optimizable in the common case of using a user-specific password. A table scan would still be needed when using the invitation password, but that's relatively rare.
Admin
It's certainly possible to have an account that's inactive, in that the user can't log in, but the account still EXISTS and could be reactivated in the future.
The bigger question some may ask is why you'd have IsDeleted instead of just actually deleting the record. Well, this is something I've used extensively: the data needs to remain in the database for potential future administrative purposes (for example, a record was modified by user 123 a year ago, and that user was since deleted from the system, but we need to know who they were), but as far as the APPLICATION is concerned, the user IS deleted.
Having to rely on digging up the user's info out of a tape backup in such a scenario is not practical.
Of course, an alternative would be to simply have a single field that accepts more than 2 states.
Admin
I may not be able to tell it for sure, but I think I can smell it: If the passwords are actually stored in a crypted form, then not only are "PSWD", "@PSWD" and "DBO.GET_INVITATION_PWD()" misnomers, but also the application server which I assume to use this stored procedure has to retrieve the random per-password salt that every good crypting algorithm uses from the database, crypt the password and only then call spAUTH. So spAUTH would also be a misnomer since it does not perform full authentication. In theory this could be how the system is implemented. In practice, I consider it unlikely that the guy who brought us the invitation password WTF would even think of using a crypting algorithm given that there is no evidence that he did, and there is some circumstantial evidence that he didn't.
Note also that if the password is crypted, then unless the application server performs some special handling for invitation passwords, the invitation password code would not work at all (not sure whether this would be a bad thing...) because the per-password salt would cause @PSWD to assume a user-dependent value even if the same invitation password was entered. spAUTH could have been implemented to handle this, but it wasn't.
Admin
The EMAIL field is a human-readable primary key here. It should be indexed, and be unique for log-on purposes. (The USER_ID field still may be the real primary key, since database engines prefer integer keys.)
There is no valid reason to index a table on PSWD! In fact, that's another security risk. The faster a hacker can guess a PSWD and get back the query, the easier it is.
Also, database servers certainly can optimize a query based on an OR. They use an index to create a set A of rows where (field = 1 OR field = 2). Compare this to the IN(1,2) predicate, which is the same, and uses indexes. This much smaller set A is then then reduced "in memory" by the other WHERE rules.
Admin
Because "Pink" doesn't describe what "IsDeleted" means?
I can understand the two flags.
And I understand needing backups too, but backups are not a solution to functionality.
Admin
Indexing the password field seems like a bad idea, if only because it's redundant, but if there is a security risk associated with it I would say it's the general possibility of timing attacks (depending on your DBMS implementation). Such an attack would probably be very difficult, but it might work.
Fast query processing in the DBMS, however, is generally desirable and brute-forcing attempts can be slowed down using other means, for example a simple sleep(), maybe even a smart sleep that adds extra delays when there have been a lot of failed login attempts in a certain time span (though not so much as to allow true DoS attacks). The sleep() has the advantage of not adding to the system load. You could also use a more expensive password crypting algorithm, which does add to system load but has the advantage of also making offline attacks more expensive should the attacker manage to retrieve the crypted passwords from the database.
Admin
Indexing the password field seems like a bad idea, if only because it's redundant, but if there is a security risk associated with it I would say it's the general possibility of timing attacks (depending on your DBMS implementation). Such an attack would probably be very difficult, but it might work.
Fast query processing in the DBMS, however, is generally desirable and brute-forcing attempts can be slowed down using other means, for example a simple sleep(), maybe even a smart sleep that adds extra delays when there have been a lot of failed login attempts in a certain time span (though not so much as to allow true DoS attacks). The sleep() has the advantage of not adding to the system load. You could also use a more expensive password crypting algorithm, which does add to system load but has the advantage of also making offline attacks more expensive should the attacker manage to retrieve the crypted passwords from the database.
Admin
Kind of a clever backdoor. I mean, unless someone actually made 2 accounts and noticed they got the same password both times, who would try it? Granted, a backdoor should be using a password that's never given to the user, so this is still a WTF.
Admin
A doctor becomes a member of our network. He sees patients, performs consultations with other doctors, has his ID associated with thousands of patient records in the DB. Then he moves away.
So, just delete him, huh?
Admin
What's wrong with that? In my Ruby on Rails apps I use the acts_as_paranoid plugin which sets deleted_at when you delete an object. When an object is deleted, it never shows up in queries (unless you explicitly include it). So it is virtually "deleted" without actually losing hte information in case you need to run reports on deleted items. And there's also an "active" like status for users so they still display on the site, but are inactive for whatever reason.
Admin
A number of apps will reset the user's info and set an isDeleted flag to indicate that there was a user. It's handy for sites where users submit content and interact.
With your method, what is the proper way to display a forum message from a deleted user? Can you still do a join to grab the deleted user's info?
Admin
The REAL WTF is that he used a UNION where an OR would have sufficed.
Admin
OMG! Yor Facebook!
Admin
The second SELECT statement might be just an accident or consequence for lack of code review. However, the way of authentication is just crab.
How should password be stored in the database as plain text? The conventional practice is to store only the hash.
Admin
If you can't log in then you should probably use frames.
Admin
Your solution doesn't work if, say, the user made a comment.. do you delete the comment? What if someone replied to that comment? Having a deleted flag is perfectly acceptable.
Admin
Admin
Database backups are no good for restoring a deleted user unless you have a backup for every single state the database was ever in, and can restore only exactly the right pieces of data - leaving all the rest of the users in tact.
Why are you worried about storing the state of a user account? It makes perfect sense to have an incremental policy of initially setting a status flag, and coming along a little while later - depending on backup frequency - to remove or achive deleted data.
I have no idea how this got to be a highlighted comment - WTF???
Admin
Ok so i see some of you are confused with the simple elegance of actually deleting a user when they are to be no longer available to the app...
So applying it to your example: you want to be able to show a treating doctors name on the patient record...well it stands to reason that when the doctor treats the patient the doctors name should be copied from the doctors record over onto the patients record...come on....its not rocket science...why would you want to go clutering up your database with old deleted data...
Oh yeah and that referential integrity thing...yeah we had some problems with that...but i solved that one as well, and significantly improved database performance at the same time....we removed all those uneccesary constraints that some noob had added.
Admin
Never heard of relational integrity? You can't just trash a user when it has other records assosiated with it wich may not be trashed (order history anyone?) Or would you just store all userinfo redundant with the orders?