Password encrypting tool

Question

I wrote a tool that takes a domain name and a password, concatenates them, hashes the result with sha512, and returns the result encoded in base64 truncated to 32 characters. The main idea behind this is to prevent an abusive server owner from getting my password by storing my password in plain text and using that password on other accounts, or if the developer forgot to remove the password from logs and a hacker got into them.

A lot of my code was already written and used in another application. The base64 encoder was taken from https://github.com/superwills/NibbleAndAHalf and slightly modified. makeSyscallError used to throw a std::system_error but to keep things simple I just made it call std::perror and exit in this code.

My main question is, is this a secure way to protect my password? I know I should really be keeping a database of randomly generated passwords and encrypt it with my master password, but I don't want to lose that database and I don't want to put the encrypted database in the cloud.

here's mpass.cpp:

#include <iostream>
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <signal.h>
#include <termios.h>
#include <unistd.h>
#include "hash.hpp"
#include "base64.h"

struct termios newTerm, oldTerm;

void setEcho(bool echo, bool icanon){
    if(echo) newTerm.c_lflag |= ECHO;
    else newTerm.c_lflag &= ~ECHO;
    if(icanon) newTerm.c_lflag |= ICANON;
    else newTerm.c_lflag &= ~ICANON;
    if(tcsetattr(0, TCSANOW, &newTerm) == -1){
        std::perror("tcsetattr");
        std::exit(1);
    }
}

//I hate it so much when my program crashes on a signal and my terminal gets screwed up, so
void cleanUp(int sn, siginfo_t *info, void *ctx){
    tcsetattr(0, TCSANOW, &oldTerm);
    if(sn != SIGINT) psiginfo(info, NULL);
    signal(sn, SIG_DFL);
    raise(sn);
    std::exit(-1); //if raise some reason didn't kill
}

//A list of signals to clean up on
const int signalsToCatch[] = {
    SIGINT, SIGQUIT, SIGILL, SIGABRT, SIGFPE, SIGSEGV, SIGPIPE, SIGALRM,
    SIGTERM, SIGUSR1, SIGUSR2, SIGBUS, SIGIO, SIGPROF, SIGSYS, SIGTRAP,
    SIGVTALRM, SIGXCPU, SIGXFSZ, SIGPWR, 0
}

/*This struct is defined in an external C file as
struct sigaction sa = {
    .sa_flags = SA_SIGINFO
};
Now sa is already filled with 0s, except for sa_flags. I wish C++ included this feature.
*/
extern struct sigaction sa;

int main(){
    if(tcgetattr(0, &oldTerm) == -1){
        std::perror("tcgetattr");
        return 1;
    }
    std::memcpy(&newTerm, &oldTerm, sizeof(struct termios));
    sa.sa_sigaction = cleanUp;
    for(int i = 0; signalsToCatch[i] != 0; ++i){
        if(sigaction(signalsToCatch[i], &sa, NULL) == -1){
            std::perror("sigaction");
            return 1;
        }
    }

    std::string dom, pass;
    (std::cout << "Enter domain: ").flush();
    std::getline(std::cin, dom);
    (std::cout << "Enter password: ").flush();
    setEcho(false, true);
    std::getline(std::cin, pass);
    setEcho(true, true);

    (std::cout << "\nPassword for " << dom << ": ").flush();

    dom += pass;
    char buf[64];
    sha512sum(dom.c_str(), dom.length(), buf);
    int useless;
    char *ret = base64(buf, 64, &useless);
    if(ret == NULL){ //Almost forgot to include this so if someone posts about this while I make this edit, don't look at them like they're stupid.
        perror("malloc");
        return 1;
    }
    ret[32] = '\n'; //I'm just gonna put the newline here
    if(write(1, ret, 33) == -1){
        std::perror("write");
        return 1;
    }
    //I could std::free(ret), but it will get freed anyway by the program exit.
    return 0;
}

//Must be defined for hash.cpp, but I wont be catching exceptions for sha512sum
//I don't want to edit hash.cpp either, as it is the same file used in another application

void makeSyscallError(const char *what){
    std::perror(what);
    std::exit(1);
}

Here's hash.hpp:

#ifndef HASH_HPP
#define HASH_HPP

#include <stddef.h>

//Args: input buffer, input buffer length, output buffer (output buffer must always be 64 bytes or more)
void sha512sum(const void *, size_t, void *);

#endif

Here's hash.cpp:

#include <sys/types.h>
#include <sys/socket.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <linux/if_alg.h>
#include <cerrno>
#include <cstring>
//I won't show misc.hpp, it's just a definition for makeSyscallError(const char *msg);
#include "misc.hpp"

static int cryptoFd = -1;

extern "C"{
    extern const struct sockaddr_alg sha512addr;
/*This is also defined in the C file: 
    const struct sockaddr_alg sha512addr = {
        .salg_family = AF_ALG,
        .salg_type = "hash",
        .salg_name = "sha512"
    };
*/
}

//This function checks if cryptoFd is equal to -1, and if it is, it will create it

static void checkCryptoFd(){
    if(cryptoFd != -1) return;
    int bindFd = socket(AF_ALG, SOCK_SEQPACKET, 0);
    if(bindFd == -1)
        makeSyscallError("Failed to create AF_ALG socket");
    if(bind(bindFd, (struct sockaddr *) &sha512addr, sizeof(struct sockaddr_alg))){
        close(bindFd);
        makeSyscallError("Failed to bind AF_ALG socket");
    }
    cryptoFd = accept(bindFd, 0, 0);
    close(bindFd);
    if(cryptoFd == -1)
        makeSyscallError("Failed to create sha512 socket");
}
//Now, I am using linux AF_ALG not for speed (I believe this usage of it would actually be slower due to syscall overhead,
//but simply because it's there and its the only interface I actually learned how to use. I'm not looking at portability in any way, and if I were, I'd rewrite this as a browser extension
void sha512sum(const void *toHash, size_t len, void *result){
    checkCryptoFd();
    for(;;){
        if(len < 128){ //Last 128 bytes to write
            if(write(cryptoFd, toHash, len) == -1)
                makeSyscallError("(odd) Failed to write to sha512 socket");
            if(read(cryptoFd, result, 64) == -1) //Get result
                makeSyscallError("(odd) Failed to read from sha512 socket");
            return; //All done!
        }
        if(send(cryptoFd, toHash, 128, MSG_MORE)){
            makeSyscallError("(odd) Failed to write to sha512 socket");
        }
        toHash += 128;
        len -= 128;
    }
}

base64.c:

#include <stdlib.h>

const static char* b64="ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" ;

// Converts binary data of length=len to base64 characters.
// Length of the resultant string is stored in flen
// (you must pass pointer flen).
//I did modify this
char* base64( const void* binaryData, int len, int *flen )
{
  const unsigned char* bin = (const unsigned char*) binaryData ;
  char* res ;

  int rc = 0 ; // result counter
  int byteNo ; // I need this after the loop

  int modulusLen = len % 3 ;
  int pad = ((modulusLen&1)<<1) + ((modulusLen&2)>>1) ; // 2 gives 1 and 1 gives 2, but 0 gives 0.

  *flen = 4*(len + pad)/3 ;
  res = (char*) malloc( *flen + 1 ) ; // and one for the null
    //Here's the modfifcations I made
/*  if( !res )
  {
    puts( "ERROR: base64 could not allocate enough memory." ) ;
    puts( "I must stop because I could not get enough" ) ;
    return 0;
  }*/
    if(!res) return NULL; //Much better
  for( byteNo = 0 ; byteNo <= len-3 ; byteNo+=3 )
  {
    unsigned char BYTE0=bin[byteNo];
    unsigned char BYTE1=bin[byteNo+1];
    unsigned char BYTE2=bin[byteNo+2];
    res[rc++]  = b64[ BYTE0 >> 2 ] ;
    res[rc++]  = b64[ ((0x3&BYTE0)<<4) + (BYTE1 >> 4) ] ;
    res[rc++]  = b64[ ((0x0f&BYTE1)<<2) + (BYTE2>>6) ] ;
    res[rc++]  = b64[ 0x3f&BYTE2 ] ;
  }

  if( pad==2 )
  {
    res[rc++] = b64[ bin[byteNo] >> 2 ] ;
    res[rc++] = b64[ (0x3&bin[byteNo])<<4 ] ;
    res[rc++] = '=';
    res[rc++] = '=';
  }
  else if( pad==1 )
  {
    res[rc++]  = b64[ bin[byteNo] >> 2 ] ;
    res[rc++]  = b64[ ((0x3&bin[byteNo])<<4)   +   (bin[byteNo+1] >> 4) ] ;
    res[rc++]  = b64[ (0x0f&bin[byteNo+1])<<2 ] ;
    res[rc++] = '=';
  }

  res[rc]=0; // NULL TERMINATOR! ;)
  return res ;
}

//I removed the decoder

The base64 encoder came as a header library. I moved it to a .c file and added this .h file (base64.h):

#ifndef BASE64_H
#define BASE64_H

#ifdef __cplusplus
#define MYEXTERN extern "C"
#else
#define MYEXTERN
#endif

//To encode, length, returned length
MYEXTERN char *base64(const void *, int, int *);

#undef MYEXTERN

#endif

I'm sure many will ask, here's the original definition of makeSyscallError(const char *):

void makeSyscallError(const char *what){
    throw std::system_error(std::make_error_code(std::errc(errno)), what);
}

Edit: forgot to mention, this code is mainly for my personal use. I will rewrite it and release it if things look good.

Answer 1

Looks pretty good. I have no experience with AF_ALG sockets, so can't comment on the usage there (but it's a relief not to have to review home-implemented crypto, so kudos for avoiding that trap!)

Most of my suggestions are somewhat style orientated, so don't feel that there are any "must do" actions here.

---

This pattern is unusual:

(std::cout << string).flush();

While there's nothing functionally incorrect, most C++ authors would include <iomanip> and then write that more poetically:

std::cout << string << std::flush;

The last flush (before printing the retrieved password) isn't needed.

---

When using std::memcpy it's easier to see that the size argument is correct if you use sizeof expression rather than sizeof (type):

std::memcpy(&newTerm, &oldTerm, sizeof newTerm);

---

Assuming that base64.c is C code rather than C++, then the return from malloc() shouldn't be cast, nor should binaryData when it's assigned to bin. And don't use all-caps for variables - convention is that they should be used for macros, to alert the reader to take special care.

---

The MYEXTERN macro is another questionable style point. Convention says to just wrap the header in an extern "C" block, which is no more code:

#ifdef __cplusplus
extern "C" {
#endif

/* definitions */

#ifdef __cplusplus
}
#endif

---

In sha512sum(), there's a special case for the last iteration of the loop - can it be reorganised so that just comes after the loop? Something like

    for (; len >= 128;  len -= 128, toHash += 128) {
        if(send(cryptoFd, toHash, 128, MSG_MORE)){
            makeSyscallError("(odd) Failed to write to sha512 socket");
        }
    }
    if (len > 0) { //Last few bytes to write
        if(write(cryptoFd, toHash, len) == -1)
            makeSyscallError("(odd) Failed to write to sha512 socket");
    }
    if (read(cryptoFd, result, 64) == -1) //Get result
        makeSyscallError("(odd) Failed to read from sha512 socket");
    return; //All done!

---

If hash.hpp is a C++ header, then prefer to include <cstdlib> to define std::size_t.

---

std::perror("malloc") might not do what you expect - malloc() doesn't set errno on failure. You might be able to test allocation failures by using ulimit to reduce the virtual memory available to the process (it will take some trial and error), or you might be able to find a debugging malloc() that can be primed to fail at the right point.

---

The code is a little inconsistent - in some places, we have if (!value) and others we explicitly if (value == nullptr). It's easier reading if we choose one style and stick with it.

---

It might be a good idea to free the allocated memory - that lets you run the code under Valgrind without having to filter false positives of memory still in use.

---

That's all for now; I might be able to return to this later.