1. 19
  1.  

  2. 4

    If I had to guess at the root cause, I’d say their biggest error was their failure to reuse an existing, thoroughly tested, constant-time comparison function. The error would not have been possible if they did.

    Write once, test like hell, use everywhere.

    1. 2

      Aside from the actual bug, what I’m really confused by is that indexOf() isn’t constant time, so… what on earth is it doing in what is supposed to be a constant time during comparison?

      1. 2

        My guess would be that whoever wrote this was either tired or unfamiliar with Java. Likely both. I can see myself make the same mistake, if (i) I don’t remember what function I’m supposed to use to index a string, and (ii) I’m using some auto-complete feature without looking at the documentation.

        Failing to look at the docs is a big mistake, but does happen from time to time. The only way to prevent this is to not have to look at the docs in the first place. In this case, factor the comparison functionality in a utility function, that is implemented once, checked thoroughly, and used everywhere.

        1. 1

          I doubt that the author used Auto-complete, they would have had a proper loop definition if they had. Java IDEs can do that since 20 years. This whole bug smells planted to me

          1. 1

            That’s a serious accusation. Perhaps we should trace the history of contribution of the account that committed this error?

            1. 2

              It may just be messy programming, either way it is bad.

              I poked a bit at their git history and the way their use of source control seems like “dump whatever accumulated into the tree”. Combining random things in one commit, commits with a comment just called “update” or “refactoring”, and messy branch merges are not really increasing my confidence in this.

              Crypto code is hard, so I hold people that write Crypto code to a higher standard than the random left-pad js module. We have a dependency on bouncy-castle at work. Not sure what to think now.

              1. 3

                We have a dependency on bouncy-castle at work. Not sure what to think now.

                If I may, dump it or fund it.

                I’ve seen how many outstanding issues and pull requests they have, they’re overwhelmed. That could explain some of the low quality. Personally, I would never consider using such a behemoth, regardless of its quality. But if your company needs it, contributing (in either engineering time or money) might be worth its while.

          2. 1

            But in a constant-time comparison function? I think this is somthing that it would not occur to you to attempt to write in the first place unless you’re already, like, awake and happy and appropriately caffeinated.