17

During a recient PCI audit the auditor said that we had major security risks because

  1. It was possible to download static resources from our website such as images css and javascript without prior authentication.
  2. Our javascript had comments in it.

Personally I think that this is not a security risk at all. The images css and javascript where not dynamically created and they contained no data on our backend, our customer details and on mechanisms.

The comments within the javascript were just simply explaining what the methods in the javascript file did. Which anyone who reads JS could have found out anyway.

How does that show "information leakage"?

Are comments within javascript really a security risk?

Wes
  • 6,697
  • 6
  • 34
  • 59
  • the first point is a security risk, but I wouldn't call it major. Javascript comments on the other hand, a security risk? That made me laugh actually. It's not optimal, that's for sure, but not a security risk. Go ahead and use http://developer.yahoo.com/yui/compressor/ It will delete comments, and all unnecessary white spaces. – Alex Jul 05 '10 at 09:01
  • 3
    The important point here is, do the comments contain anything NOT deducible from the code itself? Like how the internal servers are organized (any note of a separate database server, a server name, or anything like that), might pose a security risk. Then again, so can the code itself, if it lets you draw such conclusions. – falstro Jul 05 '10 at 09:02
  • @roe: such as this? =) http://thedailywtf.com/Articles/Client-side_PHP.aspx – David Hedlund Jul 05 '10 at 09:08
  • Frankly, your auditor is an idiot who clearly knows nothing about security. I hope for your/your company's sake that they get fired before they can do too much damage. – David X Jul 05 '10 at 09:19

5 Answers5

17

Depending on how strict the audit, downloading images etc without authentication COULD be seen as a security risk (think diagrams, charts, graphs...).

Removing comments in the javascript is like obfuscating the code: it makes it a bit harder, but still not impossible to understand what's going on. JavaScript should be seen as enhancing-only anyway, all your security should be (duplicated) at server-side. Having anyone understand what the JS does should not be considered a risk.

Konerak
  • 39,272
  • 12
  • 98
  • 118
  • I agree with the security of dynamic created resources (graphs etc) but that simply isn't the case here. We are talking about background images logo etc. – Wes Jul 05 '10 at 09:02
  • @Wes; won't those be necessary for the authentication page anyway? :) – falstro Jul 05 '10 at 09:03
  • 1
    +1. yes! if the images (say) are not publically available to anyone on the public web, they should arguably not be publically available to anyone who has the proper URL. the same definitely holds true for any static content such as documents, which are uploaded to the server where *some-but-not-all* users can download it from the website. figuring out the URL should not short-circuit the security checks. based on your second paragraph, this should not really matter for css and js files. – David Hedlund Jul 05 '10 at 09:06
  • @roe actually not in our case well not most of the resources anyway. – Wes Jul 05 '10 at 09:06
17

It depends on the content of the commentary. Because there is no way, without human intervention, to examine the content of comments to determine whether they are risky, the most efficient way to audit this is to declare all comments in client-facing source code to be risky.

The following are examples of potentially risky comments.

// doesn't really authenticate, placeholder for when we implement it.
myServer.authenticate(user,pass);

or

// don't forget to include the length, 
//the server complains if it gets NaN or undefined.
function send_stuff(stuff, length) {
...
}

or

function doSomething() {
    querystring = ""
    //querystring = "?TRACING_MODE=true&"
    ...
    //print_server_trace();
}

Another example might be if you include a source code history header, someone might be able to find some security weakness by examining the kinds of bugs that have been fixed. At least, a cracker might be able to better target his attacks, if he knows which attack vectors have already been closed.

Now, all of these examples are bad practices anyway (both the comments and the code), and the best way to prevent it is by having code reviews and good programmers. The first example is particularly bad, but innocent warnings to your team mates, like the second example, or commented-out debugging code, like the third, are the kinds of security holes that could slip through the net.

Paul Butcher
  • 6,902
  • 28
  • 39
  • Great examples. Thanks for adding these. – james.garriss Sep 13 '11 at 13:04
  • Read through this again. Basically your argument is that if the comments reveal anything about the server implementation their a risk. Makes sense. Or in other words if the comments have infomation that can't be gleaned from the client side code. – Wes May 12 '12 at 11:43
  • @Wes: That's about the size of it, yes. Another issue might be a sweary WTF comment, accusing another programmer of being a dick (of which I didn't give an example). It reveals nothing of the server implementation, but is still a dangerous information leak that could damage company reputation or facilitate a social engineering attack. – Paul Butcher May 16 '12 at 08:52
5

Without getting into if they are a security risk or not, minify your JS on production environment, this will prevent the "information leakage" and help (in some way at least) to secure the information of your website.

regarding the security risk, I don't think JS comments are a risk at all, every website content (static) can be downloaded without authentication. (unless defined otherwise)

KensoDev
  • 3,285
  • 21
  • 38
  • Are there no automatic code formatters (I've personally not encountered any workable ones yet) that can take minified code and make it readable? I think minifying is a good way for large volume traffic sites to save bandwidth, but not really relevant to security. And one ought to design an application under the assumption that the client cannot be trusted, and any attackers already fully understand your client-side code and any unencrypted client-server communication. – Lèse majesté Jul 05 '10 at 09:19
  • I didn't mean no one will be able to read the code, this is simply a fast way to remove all comments and make it alittle less readable with a glance, no more then that and you should not expect more out of this process. – KensoDev Jul 05 '10 at 09:26
5

Not if they only reveal how the code works. Any sufficiently determined person could find that out anyway.

That said, it is probably a good idea to minify the JavaScript; not because of security, but because it will reduce download times and therefore make your site a bit more responsive.

Thomas
  • 174,939
  • 50
  • 355
  • 478
  • True, but comments shouldn't normally reveal how code works. That's what the code is for. They should say why the code does what it does. – Paul Butcher Jul 05 '10 at 15:07
  • I agree but I believe the main reason for minification isn't save bandwith as most webservers now serve content gzipped, but rather to reduce parse time in the browser. – Wes Aug 23 '10 at 12:27
3

JavaScript comments can be. depends on your logic, but certainly as it is publically available, you are giving more visibility to the workings of your code.

There are other reasons for removing this as as well, such as file size, and as a result download size.

Tools such asd JSMin can help you remove the comments and perfrom a crude obfuscation of the code.

James Wiseman
  • 29,946
  • 17
  • 95
  • 158
  • I agree with JSMin and I want to build that into our builds however thats not the issue in this case. Yes the files are over verbose and could be refactored into smaller more efficent methods. (not my code) but in the end of the day we care about the security above all else. – Wes Jul 05 '10 at 09:09