342

Update 2015-07-22: The specific problems listed below have, for the most part, been remedied. More work remains to be done to clean up other instances of insecure code or bad security advice.

If anyone else finds themselves in the same situation, where there's an insecure code snippet in a high-scoring or accepted answer and your edits are being rejected by the reviewers, refer to the accepted answer. Go join room 11 and ask for help. Some of the moderators frequent this room and might be willing to help.

If anyone is unsure about whether or not a particular answer is secure, feel free to ask me (security@).


The top results for a Google Search for php encryption are:

  1. How do you Encrypt and Decrypt a PHP String?

    Before I submitted my answer, none of the existing answers were properly authenticating ciphertexts.

  2. Simplest two-way encryption using PHP

    I am unable to provide an answer because this question is closed.~ All of its advice is terrible. The accepted question uses MCRYPT_RIJNDAEL_256 as a cipher, md5($key) (yes, hex-encoded) as a key, and md5(md5($key)) as an IV. Also, once again, it uses no MAC.

    I was able to submit a new answer: https://stackoverflow.com/a/30189841/2224584

  3. How to encrypt/decrypt data in php?

    The accepted answer originally did not authenticate ciphertexts. After a brief discussion, this was quickly amended.

  4. Irrelevant (code obfuscation question)

  5. How do I encrypt a string in PHP?

    Unauthenticated encryption, yet again. It's very important that encryption libraries include message authentication in order to be secure. Eugene's answer (not the accepted one) authenticates ciphertexts.

  6. Two-way encryption: I need to store passwords that can be retrieved

    This authenticates ciphertexts with an HMAC construct, but is vulnerable to timing attacks.

    note: this has been rectified and the security issues in the answer fixed.

  7. XOR encryption -- no comment really necessary

  8. Encrypt with PHP, Decrypt with Javascript (cryptojs)

    • MCRYPT_RAND is terrible for IVs
    • Unauthenticated ciphertexts
  9. CryptoJS-specific

  10. PHP AES encrypt / decrypt

This is terrible.

Developers who come to Stack Overflow should be given better cryptography advice. Namely:

I know Stack Overflow cannot control Google's search results, but we certainly can clean up the pages that users read when they click on popular answers.

This is not a general policy question about dangerous answers, it's a call to action to replace the bad security advice that users are likely to encounter with information less likely to leave them vulnerable.

One of my issues with the current state of things is that my edits are universally being rejected because I "should have submitted them as a separate answer instead". However, I can't answer closed questions. So even if I have a correct answer that will lead users to a better approach, I can't post it.

My attempts to remedy insecure code and bad cryptography advice have been rejected because they "changed the intent" of the original answer. What should be the correct way to promote better security practices here?

14
  • 211
    Proposal: Closed answers should have <meta name="robots" content="noindex,follow" /> added to their <head /> block, then. Let's get them knocked off the front page of Google if we can't propose better solutions. Commented May 11, 2015 at 6:54
  • 7
    Well - try us and see. I think we'd all agree that dev involvement is critical to app security. Also, see meta.stackexchange.com/q/161804/154443 for proposal to remove closed questions from search results.
    – Rory Alsop
    Commented May 11, 2015 at 10:52
  • 5
    tl;dr the comments and answers, but you might want to try and get the PHP chatroom invested in fixing some of these issues. If nothing else, they can assist deleting the offending content.
    – user1228
    Commented May 11, 2015 at 14:37
  • 14
    Moderator Note: I counted at least 4 different answers and 3 separate arguments in the comments. If you'd like to argue a certain view, make it an answer. If you'd like to write an answer, do just that. The comments are not a good place to make a stand for your position.
    – George Stocker Mod
    Commented May 11, 2015 at 15:23
  • 1
    Here is an example of an answer that has been modified by others to warn of (allegedly) bad practices. Folks may wish to refer to this practice (either for or against) in their answers. Note: this Meta post suggests edits should go at the bottom.
    – halfer
    Commented May 11, 2015 at 15:30
  • 3
    This meta post is now the third result on your linked Google results page, so at least people have been warned. Commented May 11, 2015 at 16:16
  • Honestly, I think this is part of a much larger problem; that being that certain tags are lodestones for 'bad' or partially incorrect answers, PHP being one of them. But I'm not sure what can even be done about that. Commented May 12, 2015 at 5:33
  • 1
    Beware of opinions sneaking in. In see you mentioning 'authentication' a number of times, but there are plenty of use cases where authenticating the ciphertext is not required for the solution to be secure and simply has no benefit at all (no, it never hurts, but that is insufficient reason to include/require it). This includes some of the use cases in the questions you complain about.
    – Confusion
    Commented May 12, 2015 at 6:29
  • 2
    tonyarcieri.com/… Commented May 12, 2015 at 11:56
  • Your answers quite frequently link to your blog site. And what if you're wrong, and others right ?
    – TLama
    Commented May 13, 2015 at 8:45
  • 1
    If I'm wrong, please post a proof of concept and I will correct myself. Commented May 13, 2015 at 11:53
  • Hah you get a similar problem with anything to do with date times in basically any language, although that isn't a security vulnerability.
    – JonnyRaa
    Commented May 13, 2015 at 14:49
  • 3
    Congratulations, The Daily WTF has noticed this. Commented May 14, 2015 at 19:18
  • Related: meta.stackoverflow.com/q/272570/1157054
    – Ajedi32
    Commented Mar 15, 2016 at 15:40

7 Answers 7

83

I tend to agree with your notion. Lots of those "example" codes run in production environments worldwide, and it's better for everyone (not just the developer community, but everyone) if their data is stored securely.

That said, your method was a little too aggressive, and without looking at the community consensuses in place.

  • Do not be afraid to edit code in answers - Assuming the code does the same thing, only better, and you aren't contradicting the answer itself (As in, answer says "Use md5!" with an md5 example, don't edit to sha256). Editing and improving code samples is fine, and does not deviate from the original intent. Reviewers thinking that it is are wrong and there are several discussions about this already.
  • Do not edit an answer to replace it with "this answer is wrong/bad/etc" - That's just rude and not constructive. Downvote, raise the question in meta or in chat (there's a very active PHP chatroom with a lot of security conscious people). The bad answer will get revised or deleted.
  • Post your own answer with best practices - It's a good thing and I'll even reward you for doing so.

As for answering closed questions, they're closed for a reason. If you must, and you have a much superior answer you want to introduce, put it up in chat and ask it to be reopened, or flag it with a custom flag, and ask it to be reopened for the sake of answering with a much superior answer.

Don't let this discourage you! You're doing good work here!

11
  • 9
    For an answer that uses md5 incidentally, replacing md5 by sha256 is fine (assuming sha256 doesn't introduce problems like backward compatibility). For an answer that's centered on md5, downvoting is the right solution. ‘Do not edit an answer to say "this answer is wrong/bad/etc"’ — sometimes you should, not to say “this is bad”, but to say “this is harmful” (and explain why). There's a difference between bad engineering practice (downvote, comment) and actively harmful answers (where the potential for harm must be clearly conveyed). Regarding closed questions: they should be deleted then. Commented May 11, 2015 at 12:15
  • That is a comment. Not an edit. Also, just wiping out the entire current answer (as was done) is not OK. Commented May 11, 2015 at 12:17
  • 24
    @MadaraUchiha "that is a comment" - I would love to agree with you, but as the SE mantra is "comments are temporary and can (and will) be deleted without notice, at random, if somebody flags it for any reason whatsoever", that simply doesn't work. The official response to people complaining about (valid, valuable) comments being deleted is always "if it was important then it should have been part of an answer, not a comment". Thus editing in a disclaimer seems like the second best course of action - the best would be to delete the offending answer, but there's no chance that'll ever happen.
    – l4mpi
    Commented May 11, 2015 at 12:29
  • 5
    I just can't agree with that notion that it's OK to edit someone else's code: you think you have understood the intent of the answer, but maybe you haven't (how to determine who's the expert, without more elaborate counter arguments?). By doing so, you may introduce more mistake, but that's still mostly look as if it was coming from the initial author.
    – Bruno
    Commented May 11, 2015 at 13:53
  • 3
    Do not edit an answer to say "this answer is wrong/bad/etc" <- that directly goes against the recommendations in this answer, which we've been using for a long time...
    – ircmaxell
    Commented May 11, 2015 at 14:08
  • 15
    Unfortunately, I think the reason why SO works well actually works against infosec. Since answers bubble up to the top based on popular votes. Good results only bubble up iff the majority of users are knowledgeable in the field. Unfortunately infosec really is an elite field as it is an arms war.
    – Aron
    Commented May 11, 2015 at 14:13
  • @ircmaxell I've edited to clarify my point. Adding a security notice is fine. Commented May 11, 2015 at 14:13
  • 4
    @ircmaxell - It's a tricky issue. I've seen far too many people abuse edits like this to destroy answers they don't like, that compete against theirs, or that they simply disagree with. How much should we trust a lone editor? There are many cases where this has worked (the examples you point to) and many others where it has been abused. How do we vet these edits so that good answers aren't vandalized or punished? It's a difficult question.
    – Brad Larson Mod
    Commented May 11, 2015 at 17:39
  • 4
    @BradLarson perhaps this is slightly off topic, but "How much should we trust a lone editor" <-- we already do. If they have 2k rep, completely. That's how it works today anyway. Anything can be abused. But I'd rather have the power to do something about it, than just sit and talk with no good ideas. Is there potential for abuse? Sure. But I think the opposite potential (no action) is far worse...
    – ircmaxell
    Commented May 11, 2015 at 21:23
  • 3
    @ircmaxell The problem with this edits is that it confuses authorship. While readers have the option to go into the history details, few will do (and it's even worse if the answer gets quoted elsewhere). Editing answers from other users in a way that changes their meaning is misrepresenting these authors (in a good or bad way). Your answers have your name on it, you generally don't want someone else make you say something you didn't. That could almost be fraudulent. Despite the collaborative editing features, SE is geared towards answers by individuals, who get the credit or the blame.
    – Bruno
    Commented May 11, 2015 at 22:20
  • 2
    You told about rewarding good answers for highly visible security related question (whose answers are either wrong or outdated). I have one for you: stackoverflow.com/a/31443898/1090562 Commented Jul 16, 2015 at 1:30
42

We've covered this before: How to deal with highly voted answers with security vulnerabilities?

The general consensus is that if you can "fix" the answer without destroying it, do so. If not, it may be worth while adding a warning to the answer, as well as a new answer.

This doesn't always work however: https://stackoverflow.com/a/907821/338665

The top rated answer has gained more than 100 upvotes since the warning was added. The correct answer below it trails by a factor of 5:2.

I think the best approach is to contact the author of the post and see if they can fix it. Comments are a great mechanism for this, but it'd be nice if we had some PM functionality. One of the answers I wrote was included in your list in this very question. As soon as I realized it, I fixed the code. Had you reached out to me (and given me a chance to respond), all would be fine. I suspect many of the post authors are similar.

So in short, the following steps should be taken, in order:

  1. If you can fix the code without deviating from the spirit of the answer, edit away.
  2. If not, ping the author (comment, chat, etc) to see if they are willing to fix it.
  3. If not, edit in a warning and add your own answer.

Simple as that.

10
  • 1
    Seems very optimistic. Perhaps we should encourage those who wield the mighty power of mjolnir to fix the problem...
    – Aron
    Commented May 11, 2015 at 14:17
  • Per the other discussions, I take it step 4 is, "If the question is closed, bug a moderator in the PHP chatroom to temporarily open it so another answer can be posted"? Commented May 11, 2015 at 15:04
  • 3
    @Scott ping a moderator to discuss what to do. Opening may not be the best answer. But it's worth discussing.
    – ircmaxell
    Commented May 11, 2015 at 15:11
  • I am not sure raising a mod flag, for each case where someone believes there is a need for a new answer on an off-topic question, is a good approach. Moderators are swamped already, and off-topic questions are closed to new answers deliberately. "But my answer is really good/better than the others" is judged by votes, not moderators.
    – halfer
    Commented May 11, 2015 at 15:26
  • @halfer I'm not suggesting a flag, I'm suggesting pinging a mode in a chat room. Or perhaps pinging other people in the chat room who can help (especially since we have re-open powers).
    – ircmaxell
    Commented May 11, 2015 at 15:53
  • Ah, fair enough. I'm still somewhat wary of reopening questions to make exceptions, in case people start thinking that off-topic questions can be answered after all ("My input is always valuable, I'll just get this reopened for <reason>" etc).
    – halfer
    Commented May 11, 2015 at 16:02
  • 2
    @halfer agree 100%. Which is why I don't think we should recommend blind re-opening, but instead discussion. If 5 10k+ or 1+ mods agree, then it's likely worth re-opening...
    – ircmaxell
    Commented May 11, 2015 at 16:18
  • 1
    If the community has edited an answer to discourage all use, and the warning message stands for a while without the author or any other users improving the post or objecting to the warning on technical grounds, we should then consider deleting the post. Keeping around answers while telling nobody to never use them looks goofy, and if they have a lot of votes many people will probably keep using them anyway.
    – Jeremy Banks Mod
    Commented May 12, 2015 at 20:02
  • @JeremyBanks It appears other moderators believe that the answer should not be deleted. Commented May 14, 2015 at 20:46
  • @JeremyBanks well, now you are a moderator, and the other answer would rank higher if it wasn't for the checkmark, despite getting up votes every other day. Maybe is time we delete it.
    – Braiam
    Commented Feb 12, 2017 at 16:48
15

As one of the reviewers who reluctantly rejected at least one of your edits, I'd strongly recommend hashing it out in visible comments first. If you're making some arbitrary edit to a highly-visible answer with no signs of previous discussion, I'm insufficiently confident in my own cryptanalytic abilities to sign off on that.

Comment, then edit. This solves the two main problems raised in the other answers: first, that editing first is problematic in a number of ways; second, that comments disappear after a while. Well, they don't disappear immediately, so use them to get things sorted out and then record the final consensus in the answer.

2
  • 8
    If you're not confident in the subject matter of a particular edit, why not Skip rather than Reject? Hitting Skip a lot does lead to lots of audits I've noticed however.
    – samgak
    Commented May 12, 2015 at 6:02
  • 7
    @samgak: It might be more accurate to say that I'm insufficiently confident in the cryptanalytic abilities of Suggested Edit reviewers in general and myself in particular. Otherwise, yes, I hit Skip an awful lot. Commented May 12, 2015 at 6:16
7

(This is quite similar to this question.)

The first thing to do is to leave a comment and write your own alternative answer (if possible).

In general, I wouldn't edit the code in an existing answer. Editing the code in an answer that is not yours can cause three problems:

  • You're saying you know better, but maybe you don't. From a neutral point of view, it's hard to say.
  • You're effectively changing the meaning of someone else's answer, that's not what edits are for.
  • To the casual reader who does go through the edit history, it looks like it comes from the initial answerer, which I'd say is rather unethical (especially, if instead of fixing something, you introduce a mistake).

That said, if I felt sufficiently confident, I would edit the answer saying something at the top like "[Editor's note: this is potentially insecure because XYZ]": something short but sufficiently visible, and that makes it clear that the answer is disputed, and that the disagreement comes from a different user. (Users with sufficiently high reputation can see the details of +/- scores, which is useful to find out whether an answer is controversial, but many users can't see that.)

I would still consider this type of editing as a last resort after leaving a comment on the answer and given a few days to the answerer to fix it if necessary.

11
  • 3
    "You're saying you know better, but maybe you don't. From a neutral point of view, it's hard to say." OK, but this seems too much like "Have you considered that maybe ECB is a secure encryption mode?" It's not in dispute. Also, the action you're recommending "does not always work" according to ircmaxell. This answer would be better if it either disputed that; or explained why that's OK/inevitable; or said how we ought to fix it. Commented May 11, 2015 at 21:48
  • @JasonOrendorff The problem is that if you start opening the door to editing answers in a way that changes the meaning, you can end up with edit wars, in which 3rd-party readers can't know where to stand, and where the meaning up/down-vote counts becomes even more blurry. I also think this can be misleading w.r.t. the initial author, in that it makes them say something they didn't, whether it's an improvement or not (sure one can read the history in details, but few people do), it doesn't seem ethical in terms of authorship, either way.
    – Bruno
    Commented May 11, 2015 at 21:58
  • Of course, some say "the author can always roll back", but it's actually a lot to ask to expect authors to monitor all their answers to verify if someone fixed it or messed it up. This is not wikipedia.
    – Bruno
    Commented May 11, 2015 at 22:00
  • OK, but "oh don't do that, it's rude; instead try X" is a clearly inadequate answer if X also doesn't work, right? Commented May 11, 2015 at 22:00
  • @JasonOrendorff I'm not sure what you mean... What's rude?
    – Bruno
    Commented May 11, 2015 at 22:01
  • Sorry -- by "don't do that" I mean "don't edit code in other users' answers", and "instead try X" I mean "leave a comment, and then as a last resort add an editor's note". (For "X also doesn't work", see my first comment.) Commented May 11, 2015 at 22:03
  • Editing code in other users' answers is what I think you were saying is rude. (not to put words in your mouth; I just meant "rude" I as shorthand for: against the social mores around here, a socially problematic behavior.) Commented May 11, 2015 at 22:05
  • (dissatisfied look) I guess my problem here is that you seem to think this question is about someone editing code in other people's answers, and I really don't think it's about that at all. It's about a different and also very serious problem: highly visible and potentially harmful answers. Commented May 11, 2015 at 22:06
  • @JasonOrendorff It's not a matter of being rude, it's a matter of misleading the reader (almost in a fraudulent way). If a text is signed with "answer by" at the bottom, that's the first thing the reader will see. Putting words or ideas that don't come from that user without making it clear that it comes from the editor is just wrong, irrespectively of whether the author is offended or not.
    – Bruno
    Commented May 11, 2015 at 22:08
  • I think if someone were actually harmed by a very bad answer, and it became generally known, we would suddenly consider ways of more proactively hiding very bad answers. So, maybe we should imagine that harm in advance, and consider changing our tune now. It's great that SO continues to attract people who value civility and cooperation, but answers like this one (not only this one) seem too quick to dismiss the argument that bad crypto, being potentially very harmful, is a special case. I have to run right now, so can't respond further (and I didn't downvote you, just disagreeing en passant). Commented May 11, 2015 at 22:08
  • 1
    (Don't worry about downvotes). I realise this is about harmful answers. I'm just saying the editors should clearly say something like "[Editor's note: this is considered harmful, see this other answer]" at the top (and possibly leaving additional explanatory comments). In addition my first point is about potential disputes between who the "expert" is. One could say the answer is harmful, whereas the initial author could say it is not (and maybe be right). The system doesn't really have anything to prove who's right.
    – Bruno
    Commented May 11, 2015 at 22:10
6

This is a great example of topic that begs for a canonical question and answer. Pick the best question/answer/title combo (or edit one into shape), then start merging and/or closing all the other dupes to point to that canonical question. Save that link, and everytime a new one pops up, close as a duplicate of the canonical.

The merging will take a moderator, but any user can implement the closing.

13
  • 5
    The problem is that random googlers are still going to find and use the code from closed questions, no matter if they are closed as dupes. Commented May 11, 2015 at 16:23
  • 1
    @JonasCz, Yes, but they will have a link there to go to the correct answer (and maybe still have a correct answer on their question. I forgot about to put the merging stuff in my answer. will edit. Commented May 11, 2015 at 16:29
  • 3
    The other problem is that many of these questions are not exact duplicates and shouldn't be closed as such, even if they are on very similar topics.
    – Bruno
    Commented May 11, 2015 at 16:30
  • @Bruno, that's certainly a consideration. Commented May 11, 2015 at 16:32
  • 5
    @JonasCz: If not logged into SO, they'll be redirected to the dupe target silently. Commented May 11, 2015 at 18:42
  • 1
    @Nathan, That's true, but not all questions can or will be marked as duplicate, so it's not really a solution to rely on this. Commented May 11, 2015 at 20:04
  • @JonasCz: I was just addressing your point about using bad code from properly-duped questions. Commented May 11, 2015 at 20:06
  • @NathanTuggy unless the duplicate has answers, in which case you aren't redirected under any circumstance.
    – Braiam
    Commented Feb 12, 2017 at 16:50
  • @Braiam: That should be a fairly rare circumstance; that means that either answers on the target were deleted, or both questions were asked by the same user. Commented Feb 12, 2017 at 19:57
  • @NathanTuggy not that rare actually: the 71% of all questions closed as duplicated has answers.
    – Braiam
    Commented Feb 12, 2017 at 20:00
  • @Braiam: How did you work that out, and how many of those were closed as duplicates of the user's previous question? Commented Feb 12, 2017 at 20:05
  • @NathanTuggy dunno why being closed with the same user is relevant? The method is searching for duplicate:1 to obtain the absolute number of duplicates, then duplicate:1 answer:1 to search for questions that are closed as duplicate and have 1 or more answers.
    – Braiam
    Commented Feb 12, 2017 at 20:39
  • @Braiam: Ah, that's irrelevant then. I thought you were referring to duplicates whose targets had no answers, but that's obviously not the case at all. Never mind. Commented Feb 12, 2017 at 20:45
2

I'm not sure how well this would work within the tools currently available, but one possibility might be to post a self-answered (preferably community wiki) question, with a link to the self-answered question edited into a warning message on the "bad" answer.

This allows the "bad" question to stand as-is, while letting users know that there are issues with implementing code based on the "bad" answer. Those that are willing to follow the link will be educated as to the faults in the original question/answer.

I would assume for this to gain any traction, your self-answered question would need to either provide a solution that does follow best-practices, or links into questions/answers to do. This (seems to me, at least) to be the model more-or-less followed for the FAQ questions on Meta, so it would seem like there's precedent.

-3

I sometimes want to migrate questions to security.se.

What is migration and how does it work? talks about migrating to arbitrary other sites but that doesn't seem to be possible now.

Re-enabling that, or adding security.se to the close option list (at least for questions tagged crypto would address the problem.

2
  • You can always flag for mod attention, and mods can migrate it, although in the case of these questions, they probably won't do so, as these questions are on-topic for SO, and also older than 60 days (questions older than 60 days can't be migrated). IMO, migrating is not the real solution. Commented May 11, 2015 at 17:13
  • 2
    Migrating questions with/about code to Information Security or Cryptography is not appropriate.
    – Artjom B.
    Commented May 12, 2015 at 17:08

You must log in to answer this question.

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