#3841 The simultaneous use of strncpy and a length-check in client code is confusing Coverity
Closed: Fixed 2 years ago by jhrozek. Opened 2 years ago by jhrozek.

This is a quite minor issue, but we should strive for our code to be easily understandable by Coverity.

In the common.c client-side module in sss_cli_open_socket, we first have this check:

542     if (sizeof(nssaddr.sun_path) <= strlen(socket_name) + 1) {
 543         *errnop = EINVAL;
 544         return -1;
 545     }

But then, we use strncpy and we tell it to write to sizeof(nsaddr.sun_path)

strncpy(nssaddr.sun_path, socket_name, sizeof(nssaddr.sun_path));

This has two issues:
1. strncpy always writes the requested number of bytes, so the rest of the buffer is needlesly zeroed out
2. scanners like Coverity think that, because the strncpy call doesn't itself leave room for NULL-termination, that the resulting buffer can be unterminated. This is not the case, because the check above guards us from using exactly the same sizes of input and the buffer, but it's better to avoid confusing Coverity

Metadata Update from @jhrozek:
- Issue tagged with: easyfix

2 years ago

Metadata Update from @jhrozek:
- Issue set to the milestone: SSSD Patches welcome

2 years ago

Metadata Update from @atikhonov:
- Issue assigned to atikhonov

2 years ago

Relevant Covscan quote:

sssd-2.0.99/src/sss_client/common.c:549: buffer_size_warning: Calling strncpy with a maximum size argument of 108 bytes on destination array "nssaddr.sun_path" of size 108 bytes might leave the destination string unterminated.
#  547|       memset(&nssaddr, 0, sizeof(struct sockaddr_un));
#  548|       nssaddr.sun_family = AF_UNIX;
#  549|->     strncpy(nssaddr.sun_path, socket_name, sizeof(nssaddr.sun_path));
#  550|   
#  551|       sd = socket(AF_UNIX, SOCK_STREAM, 0);

Metadata Update from @jhrozek:
- Issue close_status updated to: Fixed
- Issue status updated to: Closed (was: Open)

2 years ago

Metadata Update from @jhrozek:
- Issue set to the milestone: SSSD 2.1 (was: SSSD Patches welcome)

2 years ago

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/4835

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Login to comment on this ticket.