zsmartsystems / com.zsmartsystems.zigbee

ZigBee Cluster Library Java framework supporting multiple dongles
Eclipse Public License 1.0
138 stars 87 forks source link

Insecure Zigbee Network Encryption Key Generation #1404

Closed 0xbb closed 8 months ago

0xbb commented 10 months ago

I seems to me that this projects uses the insecure Math.random method to generate the Zigbee encryption key: https://github.com/zsmartsystems/com.zsmartsystems.zigbee/blob/61460438ce445b7d8fa45a055cf00082d8e68c8e/com.zsmartsystems.zigbee/src/main/java/com/zsmartsystems/zigbee/security/ZigBeeKey.java#L247

SecureRandom should be a secure replacement for the current method.

Also see https://github.com/Koenkk/zigbee2mqtt/issues/18313 for the same mistake


I think users should regenerate existing network keys to be secure. It maybe also be good to request a CVE for this issue.

cdjackson commented 10 months ago

The key will still be random unless the same seed is used multiple times, so I don't think there is really an issue here. From my reading of the difference, the secure version generates a larger number (128 bits vs 48 bits), but as we are only generating numbers between 0 and 255, that doesn't really matter.

So, while I don't think there's a significant security issue here, I do agree it would be better to use the new version and it would be good if you wanted to provide a PR?

0xbb commented 10 months ago

I don't want to provide a PR, but below you find the new variant of the the code:

...
import java.security.SecureRandom;
...

    /**
     * Create a {@link ZigBeeKey} with a random key
     *
     * @return {@link ZigBeeKey} containing a random 128 bit key
     */
    public static ZigBeeKey createRandom() {
        SecureRandom secureRandom = new SecureRandom();
        int key[] = new int[16];
        for (int cnt = 0; cnt < 16; cnt++) {
            key[cnt] = secureRandom.nextInt(256);
        }

        return new ZigBeeKey(key);
    }
cdjackson commented 10 months ago

Thanks. I understand the code change, but thought if you could provide a PR that would be useful as I’m currently travelling for the next few weeks and can’t really do so…Sent from my iPhoneOn 19/08/2023, at 13:25, Bruno Bierbaumer @.***> wrote: I don't want to provide a PR, but below you find the new variant of the the code: ... import java.security.SecureRandom; ...

/**
 * Create a ***@***.*** ZigBeeKey} with a random key
 *
 * @return ***@***.*** ZigBeeKey} containing a random 128 bit key
 */
public static ZigBeeKey createRandom() {
    SecureRandom secureRandom = new SecureRandom();
    int key[] = new int[16];
    for (int cnt = 0; cnt < 16; cnt++) {
        key[cnt] = secureRandom.nextInt(256);
    }

    return new ZigBeeKey(key);
}

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>