[pptp-server] Is pptpd code secure?

[Matthieu PARISOT]

Hi everybody,

I have used its4 ( http://www.cigital.com/its4/ ) to audit pptpd-1.0.1
source archive;

The dump is attached is attached with the mail.
Could anyone with a good knowledge of pptpd sources check this dump and
tells us if there's something wrong?
Is it possible to an attacker to pass arguments to pptpctrl (using a
home made pptp client) ?

Thanks...
-------------- next part --------------
----------------
pptpctrl.c:177:(Very Risky) sprintf
pptpctrl.c:680:(Very Risky) sprintf
pptpd.c:609:(Very Risky) sprintf
This function is high risk for buffer overflows
Use snprintf if available, or precision specifiers, if available.
----------------
configfile.c:65:(Very Risky) sscanf
This function is high risk for buffer overflows
Use precision specifiers, or do your own parsing.
----------------
compat.c:63:(Very Risky) strcpy
compat.c:64:(Very Risky) strcpy
compat.c:91:(Very Risky) strcpy
configfile.c:67:(Very Risky) strcpy
inststr.c:27:(Very Risky) strcpy
pptpd.c:636:(Very Risky) strcpy
This function is high risk for buffer overflows
Use strncpy instead.
----------------
pptpd.c:347:(Risky) chdir
pptpd.c:373:(Risky) chdir
Can lead to process/file interaction race conditions (TOCTOU problems)
Manipulate file descriptors, not symbolic names, when possible.
----------------
pptpctrl.c:692:(Risky) execvp
pptpd.c:353:(Risky) execvp
pptpmanager.c:365:(Risky) execvp
Many potential problems.
Close all fds, clean the environment, set the umask to something good, and reset
uids before calling.
----------------
configfile.c:93:(Risky) fopen
pptpd.c:185:(Risky) fopen
pptpd.c:212:(Risky) fopen
pptpd.c:255:(Risky) fopen
pptpd.c:320:(Risky) fopen
Can be involved in a race condition if you open things after a poor check. For
example, don't check to see if something is not a symbolic link before opening
it.  Open it, then check bt querying the resulting object.  Don't run tests on
symbolic file names...
Perform all checks AFTER the open, and based on the returned object, not a
symbolic name.
----------------
pptpd.c:293:(Risky) freopen
pptpd.c:345:(Risky) freopen
pptpd.c:371:(Risky) freopen
Can be involved in a race condition if you open things after a poor check. For
example, don't check to see if something is not a symbolic link before opening
it.  Open it, then check bt querying the resulting object.  Don't run tests on
symbolic file names...
Perform all checks AFTER the open, and based on the returned object, not a
symbolic name.
----------------
compat.c:69:(Risky) open
compat.c:70:(Risky) open
Can be involved in a race condition if you open things after a poor check. For
example, don't check to see if something is not a symbolic link before opening
it.  Open it, then check bt querying the resulting object.  Don't run tests on
symbolic file names...
Perform all checks AFTER the open, and based on the returned object, not a
symbolic name.
----------------
pptpctrl.c:120:(Risky) openlog
pptpd.c:128:(Risky) openlog
pptpd.c:298:(Risky) openlog
pptpd.c:376:(Risky) openlog
Can lead to process/file interaction race conditions (TOCTOU category B)
Manipulate file descriptors, not symbolic names, when possible.
----------------
pptpd.c:319:(Risky) umask
pptpd.c:321:(Risky) umask
pptpd.c:348:(Risky) umask
pptpd.c:374:(Risky) umask
Setting a liberal umask can be bad when you exec an untrusted process.
Reset the umask to something sane before execing.
----------------
compat.c:27:(Some risk) bcopy
At risk for buffer overflows.
Make sure that your buffer is really big enough to handle a max len string.
----------------
getopt.c:981:(Some risk) getopt
getopt.c:1011:(Some risk) getopt
Depending on the lib implementation, can be a buffer overflow problem.
Truncate all str inputs to a reasonable size before calling this.
----------------
getopt1.c:78:(Some risk) getopt_long
getopt1.c:134:(Some risk) getopt_long
pptpd.c:147:(Some risk) getopt_long
Depending on the lib implementation, can be a buffer overflow problem.
Truncate all str inputs to a reasonable size before calling this.
----------------
getopt1.c:94:(Some risk) getopt_long_only
Depending on the lib implementation, can be a buffer overflow problem.
Truncate all str inputs to a reasonable size before calling this.
----------------
ctrlpacket.c:225:(Some risk) read
ctrlpacket.c:266:(Some risk) read
pptpgre.c:116:(Some risk) read
pptpgre.c:281:(Some risk) read
Be careful not to introduce a buffer overflow when using in a loop.
Make sure to check your buffer boundries.
----------------