D12937: Drop privileges when reading the salt file

[Rolf Eike Beer]

dakon requested changes to this revision.
dakon added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> pam_kwallet.c:705
> +{
> +    int readSaltPipe[2] = { -1, -1};
> +    if (pipe(readSaltPipe) < 0) {

no init necessary, either pipe overwrites it or it returns error.

> pam_kwallet.c:711
> +
> +    const int pid = fork();
> +    if (pid == -1) {

Use pid_t

> pam_kwallet.c:714
> +        syslog(LOG_ERR, "%s: Couldn't fork to read salt file", logPrefix);
> +        return NULL;
> +    } else if (pid == 0) {

leaks the pipe descriptors

> pam_kwallet.c:720
> +            syslog(LOG_ERR, "%s: could not set gid/uid/euid/egit for salt file reading", logPrefix);
> +            exit(-1);
> +        }

leaks path

> pam_kwallet.c:726
> +            syslog(LOG_ERR, "%s: Couldn't open file: %s because: %d-%s", logPrefix, path, errno, strerror(errno));
> +            exit(-1);
> +        }

leaks pipe 1 and path

> pam_kwallet.c:728
> +        }
> +        char *salt = (char*) malloc(KWALLET_PAM_SALTSIZE);
> +        memset(salt, '\0', KWALLET_PAM_SALTSIZE);

Just use  a buffer on the stack, which can even be statically initialized (i.e. no explicit memset() needed). free(path) here.

> pam_kwallet.c:730
> +        memset(salt, '\0', KWALLET_PAM_SALTSIZE);
> +        fread(salt, KWALLET_PAM_SALTSIZE, 1, fd);
> +        fclose(fd);

Check return code. I would prefer to use open()/read()/close() directly, but that can be seen as a matter of personal preferences.

> pam_kwallet.c:735
> +
> +        if (written != KWALLET_PAM_SALTSIZE) {
> +            syslog(LOG_ERR, "%s: Couldn't write the full salt file contents to pipe");

close pipe 1 here.

> pam_kwallet.c:749
> +    waitpid(pid, &status, 0);
> +    if (status == 0) {
> +        salt = (char*) malloc(KWALLET_PAM_SALTSIZE);

please use WIFEXITED and WEXITSTATUS here. The compiler is free to optimize that into a simple compare if that's what it is, but you are now relying on an implementation detail of that status code.

> pam_kwallet.c:750
> +    if (status == 0) {
> +        salt = (char*) malloc(KWALLET_PAM_SALTSIZE);
> +        const ssize_t readBytes = read(readSaltPipe[0], salt, KWALLET_PAM_SALTSIZE);

check for NULL

REPOSITORY
  R107 KWallet PAM Integration

REVISION DETAIL
  https://phabricator.kde.org/D12937

To: aacid, dakon
Cc: dakon, mgerstner, fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180525/1d89a778/attachment-0001.html>