76

I just came across this question and it scared me. The question itself is pretty bad, typical "where's the bug in this bunch of code?". But that's not what scared me.

The code is a security disaster waiting to happen. Most worrisome is the method AES that just adds the characters of a (fixed) key to the plaintext. This means someone is going to have a really, really bad day in the near future. But it has nothing to do with the question.

What am I supposed to do in a situation like this? Act like I didn't see it? I spent a few minutes writing some comments, but this question page is clearly not the correct place and I'm not even sure he wants to hear what I have to say.

I wouldn't mind spending a few more minutes of my time helping this guy, but I'm not sure if I should or how to do so. What's the standard protocol in a situation like this?

7
  • 5
    One instance of this that I've seen a number of times for PHP questions is the "md5_encrypt" function.
    – user149341
    Commented Jul 22, 2014 at 22:12
  • The question is closed, and will likely be deleted. So there's that. Commented Jul 23, 2014 at 20:56
  • 2
    hmm.. yes, lets delete the post, rinse, and repeat.. Commented Jul 24, 2014 at 3:11
  • 2
    Regarding the example question: I don't think the one answer should've been downvoted for not addressing the security concern. Considering the top 4 comments already point out to the OP and everyone viewing that there's a problem, another person echoing the same info isn't going to make any difference. The answer is helpful to answering the original question, and therefore shouldn't be downvoted.
    – Charlie
    Commented Jul 24, 2014 at 4:28
  • 3
    Would it be useful to request a "warning" feature ? When I look at an article on Wikipedia, regularly I see a flag at the top of the article "Warning: this article lacks references" or "Warning: this article is subjective"... It seems it would be useful if we had a set of such "warnings" on SO, that would appear at the top of a question or answer, "Warning: the code in this question is vulnerable to ..." and have a system to vote on such warnings much like we have for closing. Commented Jul 24, 2014 at 7:45
  • 2
    Might I note that now the question is deleted, the security problems will not be addressed? Does that scare you?
    – Michael F
    Commented Jul 24, 2014 at 11:28
  • 2
    @MatthieuM.: Probably most efficient to stick "warning, this code doesn't fully handle errors" on all questions and answers by default, then vote to take it off in the rare cases it's not applicable ;-) Commented Jul 24, 2014 at 11:29

3 Answers 3

97

A lot of people would either leave bold comments pointing out the security issue, or include mentioning the security issue as part of an answer to the question.

If you spot a security issue, you should try to point it out, if you have time to do so. It would be irresponsible not to, because not only do you let the original poster walk off with vulnerable code, but any other user who stumbles across it may unknowingly just copy and paste the insecure code into their own applications.

You mentioned that the asker may not even be interested in discussing the security issue (emphasis mine):

What am I supposed to do in a situation like this? Act like I didn't see it? I spent a few minutes writing some comments, but this question page is clearly not the correct place and I'm not even sure he wants to hear what I have to say.

If the original poster is not interested in discussing the security issue, then that's fine, don't hound the OP about it. If the OP wants to leave security holes in his code even after you try to point it out, then that's his problem, not yours.

Just leave a big bold Warning comment for future readers anyways.

7
  • 16
    Good point about future visitors. It's 4 AM here, so I guess I'll just leave my bold comment already there.
    – BoppreH
    Commented Jul 22, 2014 at 6:52
  • 1
    If the security issues are sufficiently bad, maybe add an edit to the question pointing them out at the top.
    – Richard
    Commented Jul 22, 2014 at 7:57
  • 1
    @Richard true, but you have to be careful about getting into an edit war with the OP if it turns out that they don't like it.
    – user456814
    Commented Jul 22, 2014 at 8:38
  • @Cupcake True. At which point there are other options. There is a difference between "I didn't know" and "I deny". Much depends on how bad and how obvious the issue is.
    – Richard
    Commented Jul 22, 2014 at 12:55
  • 3
    then that's his problem, not yours - Well, not quite... It's the problem for the client for whom the code is written and further down for their customers. Think about somebody not interested in security writing hotel booking system. The code author may not care, but if that system goes live and gets hacked, the unlucky people that booked with the hotel will get their personal and financial info exposed and the hotel will end up footing a large bill.
    – Aleks G
    Commented Jul 23, 2014 at 19:53
  • @AleksG see this. Besides, you've already made an attempt to point out the issue to the original poster. If he won't listen, what are you going to do, point a gun to his head and force him to comply?
    – user456814
    Commented Jul 23, 2014 at 19:56
  • 3
    @Cupcake Ha! Sometimes I wish I could...
    – Aleks G
    Commented Jul 23, 2014 at 20:05
31

The most common case of this would probably be people new to PHP trying to store user input in a database, or query a database based on user input. It's a very frightening tag, to say the least. You should definitely point out the problem, but try to use a degree of finesse when you do.

Some of the top users in PHP have gotten to the point that they burned out nearly completely due to every other question being a SQL injection train wreck. This sort of frustration can lead to belittling (albeit unwittingly) the very person you're trying to help. Just take care to be friendly, "I don't think you realize it, but you've got a pretty serious security problem there (perhaps unrelated to your question) [link]here's how to fix it[/link]" tends to work really well. "Someone from Abu Dabi is going to p0wn your crappy app" (because that happened, both the snark and the p0wning) makes folks ignore it, or flag it.

Remember, there's no diction but emphasis, so when you make use of lots of bold and such, you have to be careful to craft the voice that the comment conveys - otherwise it (like so many others) might end up getting flagged and deleted, which would be rather counterproductive :) It also helps to have a small collection of links to canonical questions and answers about the problem - also very handy for future users.

1
  • 1
    The code is not from PHP, but a local desktop application. This is why I didn't rate the SQL injection higher than the AES travesty. But I didn't realize it's a common issue in certain tags and how important the voice is.
    – BoppreH
    Commented Jul 22, 2014 at 16:43
5

Just comment -- I think your comment was fine.

I wouldn't bold the whole comment, since I wouldn't presume that my comment is the only one people should really be paying attention to. If the comment is good and the question is well-read then the comment will get upvotes (and in this case did). Upvoting is Stack Exchange's mechanism for highlighting whole comments. Not bold, not all caps, not loads of exclamation marks.

Don't scream and shout. Don't make it your life's work to persuade this person they are wrong. Don't even try to work out whether they really mean it, or if that's just a (very weird) stub implementation so that their program runs enough to demonstrate this issue that they're working on now.

For all you know (although it's unlikely), they have a unit test for that function, that is currently failing, and there is no danger at all. They can ask another question later how to implement AES (although that might well get closed too as a library request).

In fact in this case I personally probably wouldn't even comment. The code in question is so clearly not AES that I'd consider the function name to be the error. The author seems way out of their depth, and if they genuinely believe that to be an AES implementation then they're beyond my help. If you want to help them with AES then good luck to you, but that's nothing to do with the question asked :-)

It's very neighbourly of you to point out the issue, but you will kill yourself if you make it a goal to ensure all the code you see on SO is secure. Nobody is advising anybody else to use that code for any purpose: its security implications are pretty much moot. So you don't have to review question code except as it relates to the question.

I disagree with Johan's comment on the answer, "-1 because you cannot answer a question like this whilst leaving security issues unaddressed". I think you can do exactly that. Asking a question on Stack Overflow does not entitle the questioner to a free security audit (or any kind of free code review). It's legitimate merely to answer the question actually asked, if that's what you want to contribute.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .