gocd: go-cd is using an insecure block encryption algorithm.

Per config/config-api/src/com/thoughtworks/go/security/GoCipher.java, it appears that go-cd is using DES for encryption:

import org.bouncycastle.crypto.engines.DESEngine;
import org.bouncycastle.crypto.modes.CBCBlockCipher;

DES has been breakable for some time now. In addition, the CBC block cipher mode does not provide resistance against attacker tampering.

I recommend switching to AES, which is provided by org.bouncycastle.crypto.engines.AESEngine. In addition, I suggest switching to a block cipher mode that provides authenticated encryption, which prevents chosen ciphertext attacks. org.bouncycastle.crypto.modes.AEADBlockCipher provides this, and this library supports both CCM and EAX, two authenticated encryption modes.

If for some reason you cannot use the modes provided by AEADBlockCipher, you can perform an SHA256 HMAC on your CBC mode encrypted ciphertext, which I believe can be found in `org.bouncycastle.crypto.macs.HMac.

Finally, I noticed that the ciphertext of secrets are viewable in the response for the corresponding edit page. Even with properly implemented encryption, it is best to never send this data to the browser if it isn’t needed.

If you have any questions about my recommendations, please let me know.

About this issue

Original URL
State: closed
Created 9 years ago
Comments: 22 (18 by maintainers)

Most upvoted comments

> <vault>
>       <item plugin-id="vault.gpg">
>         <key>some-uuid-here-1234</key>
>         <value>someencryptedvaluehere</value>
>       </item>
>       <item plugin-id="vault.gocd">
>         <key>some-uuid-here-1234</key>
>         <value>someotherencryptedvaluehere</value>
>       </item>
>     </vault>

We might actually need to pay some attention of authorization of items in the vault, just knowing the name of the item in the vault will give access to just about everything…

ketan on Jun 23, 2017

@tomzo I think I was talking about “Case 2” in this comment. A single key-value pair. Single UUID.

What would help is a top-level tag at the <cruise> or <security> level, which holds these and can be used anywhere in the config:

<cruise>
  <security>
    <vault>
      <item plugin-id="vault.gpg">
        <key>some-uuid-here-1234</key>
        <value>someencryptedvaluehere</value>
      </item>
      <item plugin-id="vault.gocd">
        <key>anew-uuid-here-4567</key>
        <value>someotherencryptedvaluehere</value>
      </item>
    </vault>
  </security>

  <pipelines>
    <pipeline>
      <git src="https://#{CRED[some-uuid-here-1234]}:some/url/here" />
    </pipeline>
  </pipelines>
</cruise>

and only the UUID is sent back and forth during edits and is made available to be used by the config repo too.

However, first steps first:

We need to move away from the current cipher.
We can handle config-repo usage of the (old) cipher because it is separate like this. I agree with you that we need to provide a time window for migration, with suitably scary warnings.

We will also need to change the API call to provide new UUIDs or tokens or whatever we decide. Do we then need to prefix a token with something so we can figure out it is a token of a specific type or something, I wonder. Maybe not.

arvindsv on Jun 23, 2017