12692: Use of untrusted string value 12687..12691: Unbounded source buffer
git patch file (master) -- 12692 - Use of untrusted string value 0001-Ticket-47835-Coverity-12687.12692.patch
12691 - Unbounded source buffer 0002-Ticket-47835-Coverity-12687.12692.patch
12690 - Unbounded source buffer 0003-Ticket-47835-Coverity-12687.12692.patch
12689 - Unbounded source buffer 0004-Ticket-47835-Coverity-12687.12692.patch
12688 - Unbounded source buffer 0005-Ticket-47835-Coverity-12687.12692.patch
12687 - Unbounded source buffer 0006-Ticket-47835-Coverity-12687.12692.patch
Reviewed by Rich (Thank you!!)
Pushed to master: b6b7199..43c6ff2 master -> master commit 43c6ff2 commit 0a546bc commit 48f2ea0 commit 162604a commit f25c7f1 commit 8dc3806
I would like to ask what is wrong with calling strdup. It should be safe. I saw many replacement of strdup in patches. In my opinion, it is a false positive.
For example: {{{ diff --git a/ldap/servers/slapd/tools/dbscan.c b/ldap/servers/slapd/tools/dbscan.c index 023fade..bbfcd0e 100644 --- a/ldap/servers/slapd/tools/dbscan.c +++ b/ldap/servers/slapd/tools/dbscan.c @@ -1077,16 +1077,17 @@ is_changelog(char *filename)
static void usage(char argv0) { - char copy = strdup(argv0); + long arg_max = sysconf(_SC_ARG_MAX); + char copy = strndup(argv0, arg_max); }}} Variable ''argv0'' has type ''char ''. So it is nul terminated string.
The manual page says: {{{ The strdup() function returns a pointer to a new string which is a duplicate of the string s. Memory for the new string is obtained with malloc(3), and can be freed with free(3). }}}
I cannot see a reason why there should be any problem. Could you explain me it? I would like to learn something new.
Yes, it could be false positive. I'm just worried the case when the given string is corrupted/tainted (e.g., no NULL terminated).
We had this code for years and have not heard of any security issue. But we thought it was a good practice to set the maximum length to the string to duplicate not to overflow anything... At least, you see no harm on that, don't you?
I'm not sure if a string passed from an environment variable, or from a command line argument, can really be non-NULL terminated. We should do some research on that, and also possibly find out what Coverity users usually do to fix such issues.
I cannot say it was a complete research :p, but it was based upon the search result on the coverity error and fix... I don't mind undoing the changes and just mark "false positive" on coverity if that's better.
One line fix: 13ec1d6..6e175e3 master -> master commit 6e175e3 Description: arg_max has to be always set. But introduced by commit 0a546bc
It turned out these defects are false positive. Reverting... commit 43c6ff2 commit 0a546bc commit 48f2ea0 commit 162604a commit f25c7f1 commit 8dc3806 commit 6e175e3
Reverted: daf4b42..3a66ec7 master -> master {{{ commit 3a66ec7 This reverts commit 43c6ff2. commit dc4527b This reverts commit 0a546bc. commit f501430 This reverts commit 48f2ea0. commit 5065496 This reverts commit 162604a. commit cef8209 This reverts commit f25c7f1. commit 8247976 This reverts commit 8dc3806. commit 4b66c03 This reverts commit 6e175e3. }}}
Metadata Update from @nhosoi: - Issue set to the milestone: 1.3.3 backlog
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1166
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Invalid)
Login to comment on this ticket.