views:

128

answers:

5

Hi,

I am trying to write an application which amongst other things uses the openssl blowfish implementation (blowfish.h) to transport files over a simple server/client pair.

However, whilst some files are encrypted, transported, received and decrypted correctly, some end up being corrupted, after the final decryption stage. This leads me to think that the encryption routines are not being called correctly (since I have also tried with equivalent DES library calls, with the same 'intermittent corruption' results).

The relevant code is pasted below.

Basically, it starts with the function send_file (called by a connected client). This splits the file into chunks. Each 1024 byte chunk is encrypted separately and then sent. Each chunk is then received by the server in the receive_file function, decrypted and saved to disc.

Any idea what the problem could be? (Note if necessary, I will add the code for the whole application).

Cheers, Ben.

void encryptHelper(const char*,int);
void decryptHelper(const char*,int);

inline void blowfish(unsigned char *data, int data_len, char* key, int enc)
{
    //  hash the key first! 
    unsigned char obuf[20];
    bzero(obuf,20);
    SHA1((const unsigned char*)key, strlen(key), obuf);

    BF_KEY bfkey;
    int keySize = strlen(key);
    BF_set_key(&bfkey, 16, (const unsigned char*)obuf);

    unsigned char ivec[8];
    memset(ivec, 0, 8);

    unsigned char out[1024];// = (unsigned char*) malloc(1024);
    bzero(out,1024);
    int num = 0;
    BF_cfb64_encrypt(data, out, data_len, &bfkey, ivec, &num, enc);

    data=out;

    //memcpy(data, out, data_len);
    //free(out);
}
void MyFrame::encryptHelper(char* orig, int inlength)
{
char *pb=(char*)(std::string((passInput->GetValue()).mb_str()).c_str());
blowfish((unsigned char*)orig, inlength, pb, DES_ENCRYPT);
}

void MyFrame::decryptHelper(char* orig, int inlength)
{
char *pb=(char*)(std::string((passInput->GetValue()).mb_str()).c_str());
blowfish((unsigned char*)orig, inlength, pb, DES_DECRYPT);
}


int MyFrame::send_file(int fd)
{

char rec[10];
struct stat stat_buf;
fstat (fd, &stat_buf);  
int size=stat_buf.st_size;

int remSize=size;

int value=0;

while(size > 0)
{
    char buffer[1030];
    bzero(buffer,1030);
    bzero(rec,10);
    int n;
    if(size>=1024)
    {
        value+=1024;
        n=read(fd, buffer, 1024);

        // encrypt is necessary
        if(encButtonOn->GetValue()) encryptHelper(buffer,1024);

        // Send a chunk of data
        n=send(sockFile_, buffer, 1024, 0 );

        // Wait for an acknowledgement
        n = recv(sockFile_, rec, 10, 0 );
    }
    else // reamining file bytes
    {
        value+=size;
        n=read(fd, buffer, size);
        if(encButtonOn->GetValue()) encryptHelper(buffer,size);
        buffer[size]='\0';
        n=send(sockFile_,buffer, size, 0 );
        n=recv(sockFile_, rec, 10, 0 );
    }

    MyFooEvent event( 0, 992 );
    double firstBit = (double)value/remSize;
    firstBit=firstBit*100.0;
    event.adouble=firstBit;     
    wxPostEvent (this, event);  


    size -= 1024;

}

// Send a completion string
int n = send(sockFile_, "COMP",strlen("COMP"), 0 );
char buf[10];
bzero(buf,10);
// Receive an acknowledgemnt
n = recv(sockFile_, buf, 10, 0 );

return(0);
 }

 int MyFrame::receive_file()
 {

// receive file size and send ack
char sizeBuffer[50];
bzero(sizeBuffer,50);
int n;
//read(This->sockpw,buffer,bufferSize);
n=read(sockFile_, sizeBuffer, 50);
n=send(sockFile_,"OK", strlen("OK"), 0 );

int size = atoi(sizeBuffer);

//std::cout<<size<<std::endl;

// receive file name and send ack
char saveName[256];
bzero(saveName,256);
n=read(sockFile_, saveName, 256);

n=send(sockFile_,"OK",strlen("OK"), 0 );

//std::cout<<saveName_<<std::endl;

// start file writing process to local disk
// decrypt first if necessary
std::cout<<arraySize(saveName)<<std::endl;
std::cout<<strlen(saveName)<<std::endl;
if(encButtonOn->GetValue()) decryptHelper(saveName,strlen(saveName));
ofstream outFile(saveName,ios::out|ios::binary|ios::app);

// vars for status gauge
int remSize=size;
int value=0;

while(size > 0)
{       
    // buffer for storing incoming data
    char buf[1030];
    bzero(buf,1030);
    if(size>=1024)
    {

        value+=1024; // for status gauge

        // receive chunk of data
        n=recv(sockFile_, buf, 1024, 0 );

        // decrypt if necessary
        if(encButtonOn->GetValue()) decryptHelper(buf,1024);

        // write chunk of data to disk
        outFile.write(buf,1024);

        // send acknowledgement
        n = send(sockFile_, "OK", strlen("OK"), 0 );

    }
    else
    {
        value+=size;
        n=recv(sockFile_, buf, size, 0 );
        if(encButtonOn->GetValue()) decryptHelper(buf,size);
        buf[size]='\0';
        outFile.write(buf,size);
        n = send(sockFile_, "OK", strlen("OK"), 0 );
    }

    // Update status gauge
    MyFooEvent event( 0, 992 );
    double firstBit = (double)value/remSize;
    firstBit=firstBit*100.0;
    event.adouble=firstBit;
    wxPostEvent (this, event);  

    size -= 1024;

}

outFile.close();

// Receive 'COMP' and send acknowledgement
// ---------------------------------------
char buf[10];
bzero(buf,10);
n = recv(sockFile_, buf, 10, 0 );
n = send(sockFile_,  "OK", strlen("OK"), 0 );
std::cout<<"File received..."<<std::endl;

// Display image event
MyFooEvent eventF( 0, 995 );
eventF.SetText(wxString(saveName, wxConvUTF8));
wxPostEvent (this, eventF);     

return(0);
 }
+2  A: 

I'm assuming that:

char *pb=(char*)(std::string((passInput->GetValue()).mb_str()).c_str());
blowfish((unsigned char*)orig, inlength, pb, DES_DECRYPT);

decrypts into pb, which is actually the buffer of a temporary string. You simply cannot use std::string like this. The fact that you had to use so many casrs to do this shouldhave been a warning - good C and C++ code does not normally require casts at all. Basically, you need to rethink what you are doing.

anon
Thanks Neil, thats a good point about the casts, but I think you mis-understand. pb is basically the password. orig is the data. And the encrypted data is written in to orig (check blowfish function in code snipped) :-)
Ben
And not just a temporary string, but a *const* temporary string. Const is a way of specifying that the data isn't yours to fiddle with (it's constant). Casting away this is very dangerous.
Jeff Foster
So casting orig from const char* orig (as passed into function) to (unsigned char*)orig is a bad idea? I personally believe that the pb casting is not important since it is a string and not a piece of data? (But I'm guessing I'm wrong?). Cheers.
Ben
@Ben But orig is a const char * in your code - you can't write there either.
anon
hmmm. I see. Will have a look at changing this right now.
Ben
Right, I tried changing from const char* orig to char* orig, but this hasn't fixed problem...
Ben
A: 

not sure, could be a buffer overrun somewhere or memory corruption...

you could use valgrind to detect the issue or perhaps try simplifying the conversions/...

jspcal
A: 

Having fixed a few bugs by asking a few other questions, I have gotten the file encryption process working, but only when the client and server are both on the same localhost machine. When they reside on different machines, the file still ends up being corrupted. I think it is due to the fact that send_file and receive file are called from threads as follows:

void 
*MyFrame::send_fileT(void* tid)
{
accessHelper* ah = static_cast<accessHelper*>(tid);
MyFrame* This = ah->This;
This->send_file(fileSendID);
pthread_exit(NULL);
} 

void 
*MyFrame::receive_fileT(void* tid)
{
accessHelper* ah = static_cast<accessHelper*>(tid);
MyFrame* This = ah->This;
This->receive_file();
pthread_exit(NULL);
}

....and then the receive_file or send_file functions are calling the blowfish function to carry out the encryption. Now if a function is called within a pthread (i.e. send_file and receive_file), then if that function calls another function (i.e. encryptHelper -- blowfish), is it possible that the calling function will not 'properly' wait for the called function to finish correctly?

Ben
A: 

Fixed:

 n=read(fd, buffer, 2048);
 if(enc)encryptHelper(buffer,n);
 n=send(sockFile_, buffer, n, 0 );
 [called in a loop]

The problem was, was that it cannot be ensured that all n bytes of the encrypted buffer are transferred. Thus only some of the encrypted bytes are sent leading to inconsistent decryption on the receiving end.

Ben
A: 

Also you may use a checksum to verify the data integrity for faster detection of corrupt transfers.

Sud33p