How would I convert this crypto from C# to C

Question

This is the C# code I use:

public void Decrypt(byte[] @in, byte[] @out, int size)
{
    lock (this)
    {
        for (ushort i = 0; i < size; i++)
        {
            if (_server)
            {
                @out[i] = (byte)(@in[i] ^ 0xAB);
                @out[i] = (byte)((@out[i] << 4) | (@out[i] >> 4));
                @out[i] = (byte)(ConquerKeys.Key2[_inCounter >> 8] ^ @out[i]);
                @out[i] = (byte)(ConquerKeys.Key1[_inCounter & 0xFF] ^ @out[i]);
            }
            else
            {
                @out[i] = (byte)(ConquerKeys.Key1[_inCounter & 0xFF] ^ @in[i]);
                @out[i] = (byte)(ConquerKeys.Key2[_inCounter >> 8] ^ @out[i]);
                @out[i] = (byte)((@out[i] << 4) | (@out[i] >> 4));
                @out[i] = (byte)(@out[i] ^ 0xAB);
            }
            _inCounter = (ushort)(_inCounter + 1);
        }
    }
}

and this is how I converted it to work in C.

char* decrypt(char* in, int size, int server)
{
    char out[size];
    memset(out, 0, size);
    for (int i = 0; i < size; i++)
    {
        if (server == 1)
        {
            out[i] = in[i] ^ 0xAB;
            out[i] = out[i] << 4 | out[i] >> 4;
            out[i] = Key2[incounter >> 8] ^ out[i];
            out[i] = Key1[incounter & 0xFF] ^ in[i];
        }
        else if (server == 0)
        {
            out[i] = Key1[incounter & 0xFF] ^ in[i];
            out[i] = Key2[incounter >> 8] ^ out[i];
            out[i] = out[i] << 4 | out[i] >> 4;
            out[i] = out[i] ^ 0xAB;
        }
        incounter++;
    }
    return out;
}

However for some reason the C one does not work.

Link for the full C# file

Link for the full C file

Link for the C implementation

It looks like the C# source was ported from C. Oh, how fun! A game of telephone! — cdhowie, Dec 08 '10 at 17:53
What if (server == 2) ? `if(server)` is valid C code as well. This is probably not the origin of your problem, but it strikes me as odd. — Xavier T., Dec 08 '10 at 17:54
lock(this) ? http://stackoverflow.com/questions/251391/why-is-lockthis-bad — digEmAll, Dec 08 '10 at 17:57
@Xavier has a point too. The structure of the C code should be `if (server) { ... } else { ... }` — cdhowie, Dec 08 '10 at 17:58
Maybe you have to port the lock() statement as well, but the solution will depend on your platform. — codymanix, Dec 08 '10 at 18:02
I would add to the questions about `lock()` and say... Are you sure you need this synchronized? Seems like a bad design if so. You have in and out parameters specified by someone else, it should be the caller's decision if they need synchronization. — asveikau, Dec 08 '10 at 18:09
Also, it's best not to try to write your own crypto unless you're familiar with the algorithms, the language you're writing in, how to write securely in that language, and likely pitfalls in writing the crypto. Translating line-for-line can introduce subtle differences in the code, which likely wouldn't matter for most purposes, but could cause security flaws for crypto. Find a good library in whatever language you're using, and use it according to directions. — David Thornley, Dec 08 '10 at 18:19
@David, I'm not using my own crypto, I'm emulating. @asveikau, the C# code is out-dated, not going to use it in C# anymore. And the C code doesn't need locking (afaik). — Basser, Dec 08 '10 at 18:23

score 3 · Answer 1 · answered Dec 08 '10 at 17:54

3

The most glaring error I see is that you are returning a pointer to a stack-allocated array, which is going to get stomped by the next function call after decrypt() returns. You need to malloc() that buffer or pass in a pointer to a writable buffer.

answered Dec 08 '10 at 17:54

cdhowie

158,093
24
286
300

Could you help me out by using an example? This is the first project involving unmanaged code. :/ – Basser Dec 08 '10 at 17:57
1

Sure. Replace `char out[size];` with `char *out = (char*)malloc(size * sizeof(char));`. Note that the code that calls `decrypt()` will be responsible for `free()`ing the memory when it is finished with it. – cdhowie Dec 08 '10 at 17:59
@cdhowie, still not working... I edited my original post and added my implementation. – Basser Dec 08 '10 at 18:00
Why not just pass `out` as a parameter the same way that you do in the C# code? – Karl Knechtel Dec 08 '10 at 18:34
What difference would that make? – Basser Dec 08 '10 at 18:49
@Basser: Essentially, the caller is allowed to specify where to write the data. This means that the caller has the opportunity to re-use a buffer that it has already allocated. Returning a new buffer preempts any such optimization. – cdhowie Dec 08 '10 at 18:51

score 3 · Accepted Answer · answered Dec 08 '10 at 18:03

3

There was a translation error.

The C# line:

@out[i] = (byte)(ConquerKeys.Key1[_inCounter & 0xFF] ^ @out[i]);

Became:

out[i] = Key1[incounter & 0xFF] ^ in[i];

The value on the right of the xor (^) is from the wrong array.

Additionally, you are returning a stack-allocated variable, which will cause all sorts of problem.

Change:

char out[size];
memset(out, 0, size);

to:

char *out = (char*)calloc(size, sizeof(char));

answered Dec 08 '10 at 18:03

jtdubs

13,585
1
17
12

Thanks a lot! I should've found that out myself.. been looking for over an hour. Sorry for bothering all of you.. – Basser Dec 08 '10 at 18:05
No problem. When you've been staring at the same piece of code for that long, it's easy to become blind to that sort of error. Fresh eyes always help. – jtdubs Dec 08 '10 at 18:07
Actually, I just found out this didn't fix it. It made it appear to be correct. But it still isn't working. :/ – Basser Dec 08 '10 at 18:47

codymanix · Answer 3 · 2010-12-08T18:00:27.447

1

You are returning a reference to a local variable which is illegal. Either let the caller pass in an array or use malloc() to create an array inside the method.

edited Dec 08 '10 at 18:00

answered Dec 08 '10 at 17:54

codymanix

28,510
21
92
151

2

No, returning a local variable is not illegal. Returning a **pointer to a stack-allocated variable** is illegal. (Or, at the very least, undefined behavior.) Also, note the difference between "local" and "stack-allocated" -- the former includes function-scoped static variables; the latter does not. – cdhowie Dec 08 '10 at 17:55

score 1 · Answer 4 · answered Dec 08 '10 at 18:10

I also suggest turning char into unsigned char since it is more portable. If your platform assumes char is the same as signed char, the arithmetic (bit shifts, etc) will not work right.
So just specify unsigned char explicitly (use a typedef or include <stdint.h> if unsigned char seems too long-winded for you).

How would I convert this crypto from C# to C

4 Answers4