[jahvo]

It's funny, I wonder if hearing about that github bug made them check if they had committed the same mistake... only to find that they did :-)

[badloginagain]

I think I, and everyone here, should check as well. If capable, security-minded companies can make such a mistake, so can you.

[__jal]

We schedule log reviews just like we schedule backup tests. (Similar stuff gets caught during normal troubleshooting, but reviews are more comprehensive.)

It only takes one debug statement leaking to prod - it has to be a process, not an event.

[lhnz]

Why not automate this?

Create a user with an extremely unusual password and create a script that logs them in once an hour. Use another script to grep the logs for this unusual password, and if it appears fire an alert.

Security reviews are important but we should be able to automate detection of basic security failures like this.

[actsasbuffoon]

It would also be a good idea to search for the hashed version of that user’s password. It’s really bad to leak the unencrypted password when it comes in as a param, but it’s only marginally better to leak the hashed version.

[sumeno]

This only works if you automate every possible code path. If you're logging passwords during some obscure error in the login flow then an automated login very likely won't catch it.

[lhnz]

True, but it is more effective than doing nothing.

[jazzdev]

But it's not a choice of doing this or nothing. It's a choice of doing this or something else. That something else may be a better use of your time.

[Rapzid]

Log review is an awesome idea. Do you mind divulging your workplace?

[fixitnow]

Log review is done for every single project at my workplace too (Walmart Labs). So I don't think this is a novel idea. And it does not stop there. Our workplace has a security risk and compliance review process which includes reviewing configuration files, data on disk, data flowing between nodes, log files, GitHub repositories, and many other artifacts to ensure that no sensitive data is being leaked anywhere.

Any company that deals with credit card data has to be very very sure that no sensitive data is written in clear anywhere. Even while in memory, the data needs to be hashed and the cleartext data erased as soon as possible. Per what I have heard from friends and colleagues, the other popular companies like Amazon, Twitter, Netflix, etc. also have similar processes.

[Rapzid]

It's novel to me; never worked anywhere that required high level PCI compliance or that scheduled log reviews. Adhoc log review, sure. I think it's a fantastic idea regardless of PCI compliance obligations.

[baud147258]

We just realised the software I'm working on has written RSA private keys in the logs for years. Granted, it was at debug level and only when using a rarely-used functionnality, but still.

[kdbg]

For whatever its worth, I do security assessment (pentesting and the like).

Checking logs for sensitive data is a routine test when given access atleast.

Being given that access is disappointingly not routine though.

[overlordalex]

We also do log reviews, but 99% of the time they simply complain about the volume rather than the contents.

Do you enable debug logging in production? In our setup we log at info and above by default, but then have a config setting that lets us switch to debug logging on the fly (without a service restart).

This keeps our log volume down, while letting us troubleshoot when we need it. This also gives us an isolated time of increased logging that can be specifically audited for sensitive information.

[milesokeefe]

Yep, glad I read this thread. We were making the same simple mistake.

[bigiain]

We aren't.

Now.

(We caught ourselves doing it 4-5 months back, and went through _everything_ checking... Only random accident that brought it to the attention of anyone who bothered to question it too... Two separate instances by different devs of 'if (DEBUG_LEVEL = 3){ }' instead of == 3 - both missed by code reviews too...)

[jcoffland]

This is why you should turn on compiler warnings and heed them. It would have caught this.

[bch]

And consider “Yoda Notation”[0], which some people find annoying, but I found an easy hurdle to clear:

  if ( 3 = DEBUGLEVEL ) 

wouldn’t pass the the parser because you can’t assign to an rvalue.

[0] https://en.wikipedia.org/wiki/Yoda_conditions

[simias]

I know it's irrational but I really dislike Yoda notation. Every time I encounter one while reading code I have to take a small pause to understand them, I don't know why. My brain just doesn't like them. I don't think I'm the only one either, I've seen a few coding styles in the wild that explicitly disallow them.

Furthermore any decent modern compiler will warn you and ask to add an extra set of parens around assignments in conditions so I don't really think it's worth it anymore. And of course it won't save you if you're comparing two variables (while the warning will).

[foo101]

I don't think "Yoda notation" is good advice. How do you prevent mistakes like the following with Yoda notation?

  if ( level = DEBUGLEVEL )

When both sides of the equality sign are variables, the assignment will succeed. Following Yoda notation provides a false sense of security in this case.

As an experienced programmer I have written if-statements so many times in life that I never ever, even by mistake, type:

  if (a = b)

I always type:

  if (a == b)

by muscle memory. It has become a second nature. Unless of course where I really mean it, like:

  if ((a = b) == c)

[bigiain]

Yep - I pointed out that I used to do this in Perl back in '95 or so. At least one of the devs wasn't born then, none of them had ever used Perl.

(I'm not even sure how they'd ended up with a Grails configuration that'd let them do this anyway...)

[extralego]

Thanks for sharing. I am a less experienced programmer and have never seen this before. The name is so wonderful.

[mkagenius]

Apt day for discussing Yoda condition :)

[vgy7ujm]

Yoda makes code more confusing to read at a glance so I would recommend against it.

[lawl]

In this specific case DEBUGLEVEL should be a constant anyways, and thus assignment should fail, no? Also kind of denoted by being all caps.

[marssaxman]

Yes! I've been doing this in C for years. It's a little weird to read at first, but every now and then it really saves you.

[astura]

Yeah, exactly. This error shouldn't ever happen, period. All modern development tools give big fat warnings when you do this.

[FPGAhacker]

“Should” is a bad word. If you are basing a conclusion off of a “should,” you are skating on thin ice.

[icebraining]

This is just one of the many reasons why I like Python.

    > if a = 3:
           ^
    SyntaxError: invalid syntax

[mkagenius]

People (atleast me) ignore warnings quite often, they aren’t safe haven if you ask me.

[inferiorhuman]

I'm curious, how often do you actually need to print out the password in a development context?

[zimpenfish]

I've been working on authentication stuff for the last two weeks and the answer is "more than you'd like".

But luckily it's something we cover in code reviews and the logging mechanism has a "sensitive data filter" that defaults to on (and has an alert on it being off in production.)

[bognition]

seems like a bug in your platform

[OldSchoolJohnny]

What developer in their right mind would ever log a password in the first place? Are we devolving as a profession?

[Michielvv]

Can be more accidental. e.g. dumping full POST data in a more generic way (e.g. on exceptions) that happens to also be applied on the login page.