Unexpected behavior from ConstByteArrayParameter with deepCopy true

[SpaceCheetah]

I was using a ConstByteArrayParameter with
AlgorithmParameters params = MakeParameters(Name::FeedbackSize(), 1)( Name::IV(), ConstByteArrayParameter( buf, len, /*deepCopy*/ true));
Using that with AES CFB encryption, it was encrypting to a different value with the same key and iv as another library. Eventually, I found that changing deepCopy from true to false fixed it.

I looked more into it and this appears to be because with deepCopy it sets m_mark to ELEMS_MAX, while without deepCopy it doesn't. I don't understand the code enough to understand what that actually effects.

Script showing this behavior:

bool deepCopy = true;
string_view key = "0123456789abcdef";
const byte* buf = (const byte*)(&key[0]);
CFB_Mode<AES>::Encryption cipher;
AlgorithmParameters params = MakeParameters(Name::FeedbackSize(), 1)
    (Name::IV(), ConstByteArrayParameter(buf, key.size(), deepCopy));
cipher.SetKey(buf, key.size(), params);
string inout  = "Test Message";
cipher.ProcessString((byte*)(&inout[0]), inout.size());
for(unsigned char c : inout) {
    cout << (unsigned int)c << " ";
}

Output:
deepCopy = true: 54 212 37 247 60 195 31 237 151 174 121 229
deepCopy = false: 38 139 250 34 92 236 75 111 42 125 55 140

Other libraries give, for the same key, the result with deepCopy false.
Even if the deepCopy true behavior is correct and the other libraries are just wrong, a copy flag still shouldn't affect the end behavior, right?

[noloader]

Thanks @SpaceCheetah,

Even if the deepCopy true behavior is correct and the other libraries are just wrong, a copy flag still shouldn't affect the end behavior, right?

Yeah, something sounds sideways here.

ELEMS_MAX was added due to Issue 346. In the 346 issue, we could zero memory that was not used. It could cause a transient DoS if the attacker got a program to allocate a large block, like 100 MB, but only 1 KB was used. ELEMS_MAX is a way to say, "here's the number of bytes we actually used."

I don't think ELEMS_MAX is part of the problem. I think I have an idea of the problem. I'll need a few days to look at it.

What version of the library are you using?

---

This sounds like the bug we fixed at Issue 1010. The 1010 issue was against HIGHT, but it applied to any cipher or mode using AdditiveCipherTemplate::ProcessData. The bug had been around a long time. It was an intermittent bug. It caught some folks, and not others.

[SpaceCheetah]

@noloader

What version of the library are you using?

I was using 8.2.0. However, after updating to 8.5.0, the issue still occurs, exact same result for both cases.
Oh, now I see that 8.5.0 doesn't have the commit for #1010, I'll try building master.

[SpaceCheetah]

After cloning and building it as a static library, I got completely different results with deepCopy true, but exact same with deepCopy false.

deepCopy true: 95 251 243 163 245 17 230 231 202 88 30 90 (edit: it seems to give a different value each time, which definitely isn't right)
deepCopy false: 38 139 250 34 92 236 75 111 42 125 55 140

Still definitely wrong, but at least one of the changes since 8.5.0 affected something.

To me this implies that the deepCopy true behavior is the incorrect one, since false seems to be static accross versions and other languages (in particular Java's javax.crypto.Cipher.getInstance("AES/CFB8/NoPadding")) give the same result.

[SpaceCheetah]

Ok so apparently with the current master, when deepCopy is true it's running as if it has a different IV each time. That looks like it's using uninitialized memory somewhere, as there certainly isn't a call to random.

Did some more testing, if I just iterate over a ConstByteArrayParameter deepCopy doesn't change anything. Also, constructing the AlgorithmmParameters and then getting the value with params.GetValue still doesn't show this behavior. Is it with SetKey then somehow?

[noloader]

@SpaceCheetah,

Also, constructing the AlgorithmmParameters and then getting the value with params.GetValue still doesn't show this behavior. Is it with SetKey then somehow?

I believe the problem here is, AlgorithmmParameters has one std::string to hold data when copying it to the object. If you have two strings (i.e., the binary strings key and iv), then one of them gets overwritten. Or the pointer to one of them gets overwritten. (I've encountered it in the past, but I don't recall the details. I just found a way to work around it).

SetKeyWithIV will usually work as expected for encryptors and decryptors that take a key and iv. Or, I'm not aware of a case where it does not work as expected.

You usually use AlgorithmmParameters when you need to pass a parameter that does not have a class method. For example, you can set the key and iv with SetKeyWithIV. However, there's no class method that takes a feedback size, so you pass the feedback size using AlgorithmmParameters.

[SpaceCheetah]

I don't think it's overriding exactly, since this works:

string_view key = "0123456789abcdef";
const byte* buf = (const byte*)(&key[0]);
AlgorithmParameters params = MakeParameters(Name::FeedbackSize(), 1)
            (Name::IV(), ConstByteArrayParameter(buf, key.size(), deepCopy));
ConstByteArrayParameter test{};
params.GetValue(Name::IV(), test);
for(byte b : test) {
    cout << b;
}

That outputs 0123456789abcdef, as expected, whether deepCopy is true or false. I considered if it was with a move or copy constructor, but trying both operations with AlgorithmParameters doesn't mess it up.

As for working around the issue, it's fairly easy to just not use deepCopy and keep the data around, which is what I ended up doing on my project; however it seems like quite an easy issue to run into, and the solution is very non-obvious.

[SpaceCheetah]

Decided to try setting m_register to public (changed modes.h ln 127 from protected to public). Using that, I checked the value with

for(byte b : cipher.m_register) {
        std::cout << (unsigned int)b << " ";
}

I expected: 48 49 50 51 52 53 54 55 56 57 97 98 99 100 101 102
deepCopy = false: 49 50 51 52 53 54 55 56 57 97 98 99 100 101 102 114 (seems to be iterated by 1... something to do with how iv works?)
deepCopy = true: 133 112 12 178 2 0 0 80 1 112 12 178 2 0 0 101 (random each time)

[goodwinxp]

perhaps the reasons are the same as here #1042
@SpaceCheetah try to look at the places that are indicated there in the debug for your case