4
\$\begingroup\$

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;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ 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>.) \$\endgroup\$ – Quuxplusone Feb 12 '18 at 19:05
  • 2
    \$\begingroup\$ These are defined in unistd.h, according to my compiler. u_int32_t equals uint32_t, etc. \$\endgroup\$ – Nark Feb 12 '18 at 19:11
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Nark Feb 12 '18 at 22:04
  • \$\begingroup\$ @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);. \$\endgroup\$ – chux - Reinstate Monica Feb 12 '18 at 22:13
  • \$\begingroup\$ @Nofix BTW, do you agree about the bug? \$\endgroup\$ – chux - Reinstate Monica Feb 12 '18 at 22:16
  • \$\begingroup\$ 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. \$\endgroup\$ – Nark Feb 12 '18 at 22:22
  • \$\begingroup\$ I can't figure out where my code is different then the one from Wikipedia. \$\endgroup\$ – Nark Feb 12 '18 at 22:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.