Fix true positives reported by static analyzers

This commit is contained in:
Lukáš Zaoral 2022-04-06 14:11:59 +02:00 committed by Stepan Oksanichenko
parent 761bc3f7e8
commit 3c5a07bb37
5 changed files with 260 additions and 7 deletions

2
.gitignore vendored
View File

@ -1,3 +1,5 @@
tftp-hpa-5.2/
tftp-hpa-0.49.tar.bz2
/tftp-hpa-5.1.tar.bz2
/tftp-hpa-5.2.tar.bz2

1
.tftp.metadata Normal file
View File

@ -0,0 +1 @@
e7879f0bdab1fc6f226db6f6f848d58f50548de4 tftp-hpa-5.2.tar.bz2

View File

@ -7,10 +7,10 @@ diff -up tftp-hpa-0.49/tftpd/tftpd.c.tzfix tftp-hpa-0.49/tftpd/tftpd.c
+ time_t my_time = 0;
+ struct tm* p_tm;
+ char envtz[10];
+ char envtz[22];
+ my_time = time(NULL);
+ p_tm = localtime(&my_time);
+ snprintf(envtz, sizeof(envtz) - 1, "UTC%+d", (p_tm->tm_gmtoff * -1)/3600);
+ snprintf(envtz, sizeof(envtz), "UTC%+ld", (p_tm->tm_gmtoff * -1)/3600);
+ setenv("TZ", envtz, 0);
+
/* basename() is way too much of a pain from a portability standpoint */

242
tftp-hpa-5.2-covscan.patch Normal file
View File

@ -0,0 +1,242 @@
# Fix implicit declaration of function 'bsd_signal'; did you mean 'ssignal'?
#
# On sufficiently new glibc, signal with defined _DEFAULT_SOURCE is equivalent
# to bsd_signal.
--- a/config.h 2022-03-26 01:45:23.602395248 +0100
+++ b/config.h 2022-03-26 01:45:01.076491553 +0100
@@ -294,9 +294,6 @@
void *xrealloc(void *, size_t);
char *xstrdup(const char *);
-#ifndef HAVE_BSD_SIGNAL
-void (*bsd_signal(int, void (*)(int))) (int);
-#endif
#ifndef HAVE_DUP2
int dup2(int, int);
#endif
--- a/configure.in 2022-03-26 01:56:06.656577548 +0100
+++ b/configure.in 2022-03-26 01:56:04.739068636 +0100
@@ -160,7 +160,6 @@
PA_SEARCH_LIBS_AND_ADD(xmalloc, iberty)
PA_SEARCH_LIBS_AND_ADD(xrealloc, iberty)
PA_SEARCH_LIBS_AND_ADD(xstrdup, iberty)
-PA_SEARCH_LIBS_AND_ADD(bsd_signal, bsd, bsdsignal)
PA_SEARCH_LIBS_AND_ADD(getopt_long, getopt, getopt_long)
PA_SEARCH_LIBS_AND_ADD(getaddrinfo, [nsl resolv])
if $pa_add_getaddrinfo
diff --git a/tftp/main.c b/tftp/main.c
index b2f9059..d658230 100644
--- a/tftp/main.c
+++ b/tftp/main.c
@@ -305,7 +305,7 @@ int main(int argc, char *argv[])
sp->s_proto = (char *)"udp";
}
- bsd_signal(SIGINT, intr);
+ signal(SIGINT, intr);
if (peerargc) {
/* Set peer */
@@ -768,7 +768,7 @@ void intr(int sig)
{
(void)sig; /* Quiet unused warning */
- bsd_signal(SIGALRM, SIG_IGN);
+ signal(SIGALRM, SIG_IGN);
alarm(0);
siglongjmp(toplevel, -1);
}
diff --git a/tftp/tftp.c b/tftp/tftp.c
index d15da22..52f5be0 100644
--- a/tftp/tftp.c
+++ b/tftp/tftp.c
@@ -85,7 +85,7 @@ void tftp_sendfile(int fd, const char *name, const char *mode)
is_request = 1; /* First packet is the actual WRQ */
amount = 0;
- bsd_signal(SIGALRM, timer);
+ signal(SIGALRM, timer);
do {
if (is_request) {
size = makerequest(WRQ, name, dp, mode) - 4;
@@ -191,7 +191,7 @@ void tftp_recvfile(int fd, const char *name, const char *mode)
firsttrip = 1;
amount = 0;
- bsd_signal(SIGALRM, timer);
+ signal(SIGALRM, timer);
do {
if (firsttrip) {
size = makerequest(RRQ, name, ap, mode);
# Fix leaked_handle: Handle variable "fd" going out of scope leaks the handle.
diff --git a/tftpd/tftpd.c b/tftpd/tftpd.c
index 364e7d2..cbd6093 100644
--- a/tftpd/tftpd.c
+++ b/tftpd/tftpd.c
@@ -1505,6 +1505,7 @@ static int validate_access(char *filename, int mode,
if (mode == RRQ) {
if (!unixperms && (stbuf.st_mode & (S_IREAD >> 6)) == 0) {
+ close(fd);
*errmsg = "File must have global read permissions";
return (EACCESS);
}
@@ -1514,6 +1515,7 @@ static int validate_access(char *filename, int mode,
} else {
if (!unixperms) {
if ((stbuf.st_mode & (S_IWRITE >> 6)) == 0) {
+ close(fd);
*errmsg = "File must have global write permissions";
return (EACCESS);
}
@@ -1522,6 +1524,7 @@ static int validate_access(char *filename, int mode,
#ifdef HAVE_FTRUNCATE
/* We didn't get to truncate the file at open() time */
if (ftruncate(fd, (off_t) 0)) {
+ close(fd);
*errmsg = "Cannot reset file size";
return (EACCESS);
}
# Fix warnings about useless inline in int usage(int)
From 8ddf0d87d7463c21e28dd2bea6f3f42d4c92cb1d Mon Sep 17 00:00:00 2001
From: "H. Peter Anvin" <hpa@zytor.com>
Date: Sat, 7 Jun 2014 13:00:46 -0700
Subject: [PATCH] tftp: drop "inline" from definition of usage()
It is pointless and newer gcc say it is a lose.
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
tftp/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tftp/main.c b/tftp/main.c
index 1b8a881..b2f9059 100644
--- a/tftp/main.c
+++ b/tftp/main.c
@@ -188,7 +188,7 @@ char *xstrdup(const char *);
const char *program;
-static inline void usage(int errcode)
+static void usage(int errcode)
{
fprintf(stderr,
#ifdef HAVE_IPV6
--
2.35.1
# Fix Calling "setsockopt" without checking return value. This library
# function may fail and return an error code.
diff --git a/tftpd/recvfrom.c b/tftpd/recvfrom.c
index d7ef500..e0074d8 100644
--- a/tftpd/recvfrom.c
+++ b/tftpd/recvfrom.c
@@ -26,6 +26,7 @@
#if defined(HAVE_RECVMSG) && defined(HAVE_MSGHDR_MSG_CONTROL)
+#include <syslog.h>
#include <sys/uio.h>
#ifdef IP_PKTINFO
@@ -151,16 +151,19 @@ myrecvfrom(int s, void *buf, int len, unsigned int flags,
/* Try to enable getting the return address */
#ifdef IP_RECVDSTADDR
if (from->sa_family == AF_INET || !from->sa_family)
- setsockopt(s, IPPROTO_IP, IP_RECVDSTADDR, &on, sizeof(on));
+ if (setsockopt(s, IPPROTO_IP, IP_RECVDSTADDR, &on, sizeof(on)) == -1)
+ syslog(LOG_ERR, "cannot setsockopt IP_RECVDSTADDR %m");
#endif
#ifdef IP_PKTINFO
if (from->sa_family == AF_INET || !from->sa_family)
- setsockopt(s, IPPROTO_IP, IP_PKTINFO, &on, sizeof(on));
+ if (setsockopt(s, IPPROTO_IP, IP_PKTINFO, &on, sizeof(on)) == -1)
+ syslog(LOG_ERR, "cannot setsockopt IP_PKTINFO %m");
#endif
#ifdef HAVE_IPV6
#ifdef IPV6_RECVPKTINFO
if (from->sa_family == AF_INET6 || !from->sa_family)
- setsockopt(s, IPPROTO_IPV6, IPV6_RECVPKTINFO, &on, sizeof(on));
+ if (setsockopt(s, IPPROTO_IPV6, IPV6_RECVPKTINFO, &on, sizeof(on)) == -1)
+ syslog(LOG_ERR, "cannot setsockopt IPV6_RECVPKTINFO %m");
#endif
#endif
bzero(&msg, sizeof msg); /* Clear possible system-dependent fields */
diff --git a/tftpd/tftpd.c b/tftpd/tftpd.c
index 364e7d2..36d6dec 100644
--- a/tftpd/tftpd.c
+++ b/tftpd/tftpd.c
@@ -224,7 +224,9 @@ static void pmtu_discovery_off(int fd)
#if defined(IP_MTU_DISCOVER) && defined(IP_PMTUDISC_DONT)
int pmtu = IP_PMTUDISC_DONT;
- setsockopt(fd, IPPROTO_IP, IP_MTU_DISCOVER, &pmtu, sizeof(pmtu));
+ if (setsockopt(fd, IPPROTO_IP, IP_MTU_DISCOVER, &pmtu, sizeof(pmtu)) == -1)
+ syslog(LOG_ERR, "cannot setsockopt IP_MTU_DISCOVER to "
+ "IP_PMTUDISC_DONT %m");
#endif
}
# Fixes negative_returns: "fd" is passed to a parameter of pmtu_discovery_off
# that cannot be negative
From 0b44159b3a2f51d350f309d3f6d14a17e74e8231 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Zaoral?= <lzaoral@redhat.com>
Date: Wed, 6 Apr 2022 09:33:33 +0200
Subject: [PATCH 1/2] tftpd: Correctly disable path MTU discovery in
standalone mode
---
tftpd/tftpd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tftpd/tftpd.c b/tftpd/tftpd.c
index 364e7d2..00fa1cf 100644
--- a/tftpd/tftpd.c
+++ b/tftpd/tftpd.c
@@ -769,7 +769,7 @@ int main(int argc, char **argv)
}
/* Disable path MTU discovery */
- pmtu_discovery_off(fd);
+ pmtu_discovery_off(fdmax);
/* This means we don't want to wait() for children */
#ifdef SA_NOCLDWAIT
--
2.35.1
From 5f60355c4bd10b866847a0d58a9582bda7db72aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Zaoral?= <lzaoral@redhat.com>
Date: Wed, 6 Apr 2022 09:34:46 +0200
Subject: [PATCH 2/2] tftpd: Fix a possible usage of -1 file descriptor in
standalone mode
---
tftpd/tftpd.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tftpd/tftpd.c b/tftpd/tftpd.c
index 00fa1cf..afd595d 100644
--- a/tftpd/tftpd.c
+++ b/tftpd/tftpd.c
@@ -622,6 +622,13 @@ int main(int argc, char **argv)
exit(EX_USAGE);
}
ai_fam = AF_INET6;
+
+ if (fd6 < 0) {
+ syslog(LOG_ERR,
+ "IPv6 was disabled but address %s is in address "
+ "family AF_INET6", address);
+ exit(EX_USAGE);
+ }
}
break;
#endif
--
2.35.1

View File

@ -1,10 +1,9 @@
%global systemctl_bin /usr/bin/systemctl
%global _hardened_build 1
Summary: The client for the Trivial File Transfer Protocol (TFTP)
Name: tftp
Version: 5.2
Release: 36%{?dist}
Release: 37%{?dist}
License: BSD
URL: http://www.kernel.org/pub/software/network/tftp/
Source0: http://www.kernel.org/pub/software/network/tftp/tftp-hpa/tftp-hpa-%{version}.tar.bz2
@ -23,10 +22,13 @@ Patch9: tftp-doc.patch
Patch10: tftp-enhanced-logging.patch
Patch11: tftp-hpa-5.2-gcc10.patch
Patch12: tftp-rewrite-macro.patch
Patch13: tftp-hpa-5.2-covscan.patch
BuildRequires: autoconf
BuildRequires: gcc
BuildRequires: make
BuildRequires: gcc
BuildRequires: readline-devel autoconf systemd-units
BuildRequires: readline-devel
BuildRequires: systemd-rpm-macros
%description
The Trivial File Transfer Protocol (TFTP) is normally used only for
@ -63,6 +65,7 @@ systemd socket activation, and is disabled by default.
%patch10 -p1 -b .logging
%patch11 -p1 -b .gcc10
%patch12 -p1 -b .rewrite-macro
%patch13 -p1 -b .covscan
%build
autoreconf
@ -70,7 +73,6 @@ autoreconf
make %{?_smp_mflags}
%install
rm -rf ${RPM_BUILD_ROOT}
mkdir -p ${RPM_BUILD_ROOT}%{_bindir}
mkdir -p ${RPM_BUILD_ROOT}%{_mandir}/man{1,8}
mkdir -p ${RPM_BUILD_ROOT}%{_sbindir}
@ -105,6 +107,12 @@ install -p -m 644 %SOURCE2 ${RPM_BUILD_ROOT}%{_unitdir}
%{_unitdir}/*
%changelog
* Wed Apr 06 2022 Lukáš Zaoral <lzaoral@redhat.com> - 5.2-37
- Review and fix issues reported by static analysers (rhbz#2069613)
- Use systemd-rpm-macros and modernise the specfile a bit
- Based on changes made by Dominik 'Rathann' Mierzejewski in Fedora.
Thanks a lot!
* Wed Mar 23 2022 Lukáš Zaoral <lzaoral@redhat.com> - 5.2-36
- Fix inconsistent --map-file option spelling in manual (rhbz#2066855)
- Fix memory corruption in tftpd when filename remapping with macro \x