Potential Buffer Overrun

[mloit]

In both SECP256k1.swift and Data+Extension.swift there is a section of code to initialize a buffer with random data.

This is the excerpt form Data+Extension, though the code in SECP256k1 is virtually identical

    static func randomBytes(length: Int) -> Data? {
        for _ in 0...1024 {
            var data = Data(repeating: 0, count: length)
            let result = data.withUnsafeMutableBytes { (body: UnsafeMutableRawBufferPointer) -> Int32? in
                if let bodyAddress = body.baseAddress, body.count > 0 {
                    let pointer = bodyAddress.assumingMemoryBound(to: UInt8.self)
                    return SecRandomCopyBytes(kSecRandomDefault, 32, pointer)
                } else {
                    return nil
                }
            }
            if let notNilResult = result, notNilResult == errSecSuccess {
                return data
            }
        }
        return nil
    }

The problem is at the line of calling SecRandomCopyBytes, it is being called with a fixed count of 32, instead of the input length, as a result anything less that 32 bytes there will be an overrun beyond the end of the buffer, conversely if length > 32 only the first 32bytes will get initialized [while safe, probably not the desired outcome]

An overrun actually appears to happen with this code inside EthereumKeystoreV3 (perhaps other places as well)

   fileprivate func encryptDataToStorage(_ password: String, keyData: Data?, dkLen: Int = 32, N: Int = 4096, R: Int = 6, P: Int = 1, aesMode: String = "aes-128-cbc") throws {
        if (keyData == nil) {
            throw AbstractKeystoreError.encryptionError("Encryption without key data")
        }
        let saltLen = 32;
        guard let saltData = Data.randomBytes(length: saltLen) else {
            throw AbstractKeystoreError.noEntropyError
        }
        guard let derivedKey = scrypt(password: password, salt: saltData, length: dkLen, N: N, R: R, P: P) else {
            throw AbstractKeystoreError.keyDerivationError
        }
        let last16bytes = Data(derivedKey[(derivedKey.count - 16)...(derivedKey.count - 1)])
        let encryptionKey = Data(derivedKey[0...15])
        guard let IV = Data.randomBytes(length: 16) else {
            throw AbstractKeystoreError.noEntropyError
        }

.
.
.

in the above section, the 1st call to Data.randomBytes() is safe,as the buffer is 32 bytes, but on the 2nd call the buffer is only 16 (or at least the applicable portion), resuting in the 16bytes beyond the buffer being written to.