Tiny Encryption Algorithm implementation in C

Question

I was just looking for a general feedback about this code, which encrypts a file with the Tiny Encryption Algorithm. I'm currently studying C, and I wanted to know if there was anything I could improve in my coding style or anything. Sorry for my english.

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <sys/stat.h>
#include <fcntl.h>
#define BASE_DELTA 0x9e3779b9
#define KEY_VALUE 0x7A24432646294A404E635266556A586E //128bits value
void encrypt (u_int32_t* v, u_int32_t* k) {
 u_int32_t delta = BASE_DELTA * 1;
 for (int i = 1; i <= 32; i++) {
 v[0] += (v[1] * delta) ^ ((v[1] << 4) + k[0]) ^ ((v[1] >> 5) + k[1]);
 v[1] += (v[0] * delta) ^ ((v[0] << 4) + k[2]) ^ ((v[0] >> 5) + k[3]);
 delta += BASE_DELTA;
 }
}
void encryptFile(const char file_to_encrypt_path[], u_int32_t key[4]){
 u_int32_t block[2];
 int nb_bytes_read = 0;
 int fd_to_encrypt = open(file_to_encrypt_path, O_RDONLY, 0655); //RDONLY permissions are 0655
 int fd_result_file = open("encrypted_file.dec", O_CREAT|O_TRUNC|O_WRONLY, 0666);
 while(1){
 memset(block, 0x0, sizeof(u_int64_t));
 nb_bytes_read = read(fd_to_encrypt, block, sizeof(u_int64_t));
 if(nb_bytes_read == -1){
 perror("read");
 exit(errno);
 }
 if(nb_bytes_read == 0){
 break; //EOF
 }
 encrypt(block, key);
 if(write(fd_result_file, block, sizeof(u_int64_t)) ==-1){
 perror("write");
 exit(errno);
 }
 }
}
int main(int argc, char const *argv[]) {
 if (argc < 2){
 printf("usage : %s <file to encrypt> \n", argv[0]);
 exit(EXIT_FAILURE);
 }
 u_int32_t key[4];
 u_int32_t block[2];
 memset(key, 0x0, sizeof(u_int32_t) * 4);
 memset(block, 0x0, sizeof(u_int32_t) * 2);
 __uint128_t key_val = KEY_VALUE; //Note that __uint128_t requiers gcc version > 4.4
 *key = key_val;
 encryptFile(argv[1], key);
 return 0;
}

• Quuxplusone: You should provide complete, compilable code, which means including the typedefs for u_int32_t, u_int64_t, and so on. (Or just use the standard uint32_t etc. located in <stdint.h>.)
• Nark: These are defined in unistd.h, according to my compiler. u_int32_t equals uint32_t, etc.

Answer 1

Good formatting and layout.

Aside from bug, good code.

Bug?

I'd expect v[1] + delta, v[0] + delta per Tiny Encryption Algorithm.

// Reference code
// for (i=0; i < 32; i++) { /* basic cycle start */
// sum += delta;
// v0 += ((v1<<4) + k0) ^ (v1 + sum) ^ ((v1>>5) + k1);
// v1 += ((v0<<4) + k2) ^ (v0 + sum) ^ ((v0>>5) + k3);
// } 
 for (int i = 1; i <= 32; i++) {
 // v--- ???
 v[0] += (v[1] * delta) ^ ((v[1] << 4) + k[0]) ^ ((v[1] >> 5) + k[1]);
 v[1] += (v[0] * delta) ^ ((v[0] << 4) + k[2]) ^ ((v[0] >> 5) + k[3]);
 delta += BASE_DELTA;
 }

Questionable sample code.

*key = key_val; only sets the first element of key[4], Certainly the below functionality was desired.

// *key = key_val;
key[0] = key_val >> 0;
key[1] = key_val >> 32;
key[2] = key_val >> 64;
key[3] = key_val >> 96;

Use standard types

Use standard types from #include <stdint.h> like uint32_t instead of u_int32_t. They are better known and more portable.

---

Minor

const

Use const as able to convey code's intent.

// void encrypt(u_int32_t* v, u_int32_t* k) {
void encrypt(u_int32_t* v, const u_int32_t* k) {
// void encryptFile(const char file_to_encrypt_path[], u_int32_t key[4]) {
void encryptFile(const char file_to_encrypt_path[], const u_int32_t key[4]) {

Make constants unsigned

Make constants unsigned for unsigned variables by adding u.

// #define BASE_DELTA 0x9e3779b9
#define BASE_DELTA 0x9e3779b9u

Unneeded *1

Perhaps there is some reason for this, - I do not see it.

// u_int32_t delta = BASE_DELTA * 1;
u_int32_t delta = BASE_DELTA;

Debug code?

Clearing the block serves little aside from cleaner debug. Don't use a type unrelated to the variable.

// memset(block, 0x0, sizeof(u_int64_t));
memset(block, 0x0, sizeof block);

0 base

Starting from 0 is more C-ish, yet I have no real issue with OP's code.

// for (int i = 1; i <= 32; i++) {
for (int i = 0; i < 32; i++) {

Avoid naked magic numbers

TEA uses 64 rounds, nice to self-document that in code

#define TEA_ROUNDS 64
for (int i = 0; i < TEA_ROUNDS/2; i++) {

open() test, close()?

Missing tests if open() success.
Missing close(), x2, in encryptFile().

---

Advanced

restrict

Use restrict (C99) to let encrypt() know that v, k do not overlap. This allows for some optimizations.

// void encrypt(u_int32_t* v, u_int32_t* k) {
void encrypt(u_int32_t* restrict v, const u_int32_t* restrict k) {

Out of range constant.

0x7A24432646294A404E635266556A586E is out of range of most compilers, even when __uint128_t exists.

Portable code would use a different way to set the key as well and a different type. Good that it work for you.

• Nark: Thanks a lot for all this review ! That was very instructive! Do you know any good and clean way of putting this 128bit number into my key array?
• chux: @Nofix See Numbers bigger than 8 bytes in C. Also look for other like posts on SO. A way is to use __uint128_t x = ((__uint128_t) 0x64BitUpperHalf << 64) | 0x64BitLowerHalf);.
• chux: @Nofix BTW, do you agree about the bug?
• Nark: For the key, yea, but for the algorithm, I'll need to check it again and pay more attention to it, as I made mine in classroom with a teacher. Looks like both my version and Wikipedia's one are reversible anyway. But you should be right, additionning sum is looking like to be the right way to code TEA.
• Nark: I can't figure out where my code is different then the one from Wikipedia.