Why do people even bother to try writing encryption algorithms?

[Mark R]

I was recently reviewing an electronic medical records and telemedicine package that I was planning to buy for a small remote clinic. The vendor kindly supplied me with a demo copy of the package so that I could evaluate it.

All looked good. I enquired about security - AES 256 throughout, the CEO told me.

Anyway curiosity got the better of me and I fired up my JustDecompile and decided to take a look.

public static string encrypt(string encrypt)
{
    string str;
    string empty = string.Empty;
    try
    {
        string str1 = encrypt.TrimEnd(new char[0]);
        char[] charArray = str1.ToCharArray();
        int num = 0;
        while (true)
        {
            bool length = num < (int)charArray.Length;
            if (!length)
            {
                break;
            }
            int num1 = Convert.ToInt32(charArray[num]) + 10;
            charArray[num] = Convert.ToChar(num1);
            empty = string.Concat(empty, charArray[num].ToString());
            num++;
        }
        str = empty;
    }
    catch (Exception exception)
    {
        str = null;
    }
    return str;
}

Bwahahahah.

 

[Markbnj]

Oh man, you have to be kidding me. Are you sure that's the routine they're calling? Maybe that's old code.

 

[degibson]

Wow. If they're actually calling that routine... Ignorance? Incompetence? Dishonesty? Malice?

 

[IronWing]

I'm not familiar with the language but if I'm reading it correctly it is a ROT 10 routine?

Edit: Does that {str = null} at the end just empty the string no matter what?

 

[KentState]

I'm not familiar with the language but if I'm reading it correctly it is a ROT 10 routine?

Edit: Does that {str = null} at the end just empty the string no matter what?

str = null is only if the function has an error. The try/catch statement encapsulates the logic.

 

[IronWing]

str = null is only if the function has an error. The try/catch statement encapsulates the logic.

Thanks, got it, went back and counted {}s.

 

[PhatoseAlpha]

Wow. Took me a bit there to recognize that it wasn't as completely pointless as it first seemed, since str empty was not in fact empty.

And then on third glance, it is as completely pointless as it first seemed.

Sirius Cybernetics style code - the superficial flaws hide the fundamental ones.

 

[tech7]

I wouldn't even call that encryption, it's closer to basic encoding than an actual encryption algorithm, offers almost no security at all.

 

[BRcr]

This could be just a part of encryption. If that's something like an additional encryption before AES starts, it falls in place...

 

[Cogman]

This could be just a part of encryption. If that's something like an additional encryption before AES starts, it falls in place...

Pre-encryption is a bad idea. It can introduce security flaws that are not present in the original algorithm See http://security.stackexchange.com/questions/19911/crime-how-to-beat-the-beast-successor

 

[BRcr]

Only if attacker knows of it. Anyway, pre-encryption and pre-compression are quite a different actions.

 

[Ken g6]

It took me awhile to realize that str=empty did not assign str="".

Once I got past that, I realized this is a really bad encryption method. One might even call its result ROT-TEN encryption.

 

[Cogman]

Only if attacker knows of it. Anyway, pre-encryption and pre-compression are quite a different actions.

First
http://en.wikipedia.org/wiki/Security_through_obscurity

Second. It depends. At best, it adds no additional security at worst it decreases security. It will almost never increase security.

 

[KB]

It does amaze me because .net has hundreds of pre-written, pre-tested encryption algos already that you don't need to write any code for and probably still perform better than this one.

I would double-check to make sure that is the function they are using and that you can decrypt some data using the reverse algo. Its possible that is old code as markbnj mentions.

The big problem is that if you mention this to the CEO, he is likely to sue you under DMCA. You may need to send an anonymous letter instead.

 

[Mark R]

They definitely use this function for some stuff (mainly storing some stuff on the client computer), but a more thorough inspection shows that they do indeed use AES256 for transmitting some data, with others bits of data encrypted with 3DES (mainly the software license/serial).

I didn't get a copy of the server software so don't know how it's stored on the server.

Personally, rather than encrypting bits of transmitted data manually - I'd have just used a TLS connection. Considering that the whole thing was written in .NET and used the WCF transparent client/server API, TLS support was literally a matter of changing the http:// to https:// in the service definition and attaching a self-signed certificate.

 

[Markbnj]

Personally, rather than encrypting bits of transmitted data manually - I'd have just used a TLS connection. Considering that the whole thing was written in .NET and used the WCF transparent client/server API, TLS support was literally a matter of changing the http:// to https:// in the service definition and attaching a self-signed certificate.

True, and that will protect the data in transit, but this algorithm may have been intended to encrypt data at the field level for storage purposes.

 

[Leros]

People rewrite existing code all the time. One of the most useful things I've done was to quickly look at the documention for a bunch of common libraries (Google, IBM, Apache, etc). It's saved me a ton of time. Just today, I needed to generate random strings and I used RandomStringUtils from Apache commons-lang.

 

[Apathetic]

Wow. If they're actually calling that routine... Ignorance? Incompetence? Dishonesty? Malice?

How about E) All of the above?

Dave

 

[KB]

True, and that will protect the data in transit, but this algorithm may have been intended to encrypt data at the field level for storage purposes.

If they wanted to encrypt at the storage level they should have used database encryption, not this messy function. If the database didn't support encryption they could have still used encryption functions to encrypt the strings before putting them in the database.

 

[Markbnj]

If they wanted to encrypt at the storage level they should have used database encryption, not this messy function.

Well yeah, no kidding . I was just pointing out that TLS doesn't cover security across the whole information domain. My implicit assumption was that the method above could not possibly be intended to secure information across the transport layer, because nobody could be that stupid.