Ubuntu
net-snmp package

Merge ~sergiodj/ubuntu/+source/net-snmp:double-free-bug1877027-focal into ubuntu/+source/net-snmp:ubuntu/focal-devel

  1. Git
  2. lp:~sergiodj/ubuntu/+source/net-snmp
  3. double-free-bug1877027-focal
  4. Merge into ubuntu/focal-devel
Proposed by Sergio Durigan Junior
Status: Rejected
Rejected by: Andreas Hasenack
Proposed branch: ~sergiodj/ubuntu/+source/net-snmp:double-free-bug1877027-focal
Merge into: ubuntu/+source/net-snmp:ubuntu/focal-devel
Diff against target: 872 lines (+820/-0)
8 files modified
debian/changelog (+21/-0)
debian/patches/fix-usmStateReference-free.patch (+31/-0)
debian/patches/introduce-refcount-usmStateReference.patch (+169/-0)
debian/patches/move-free-securityStateRef-into-snmp-free-pdu.patch (+92/-0)
debian/patches/move-securityStateRef-into-free_securityStateRef.patch (+78/-0)
debian/patches/prevent-snmpv3-bulkget-errors-double-free.patch (+349/-0)
debian/patches/series (+6/-0)
debian/patches/unexport-struct-usmStateReference.patch (+74/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack Pending
Canonical Server Pending
Review via email: mp+386283@code.launchpad.net

Description of the change

This series of patches fix a segmentation fault that occurs with snmpd on focal when the user requests an erroneous bulkget operation using the snmpv3 protocol. It took me a while to figure out the right set of patches that needed to be backported in order to address this issue, mostly because this is my first rodeo with snmp but also because upstream addressed this using different bug numbers.

To reproduce the issue, one can do:

$ lxc launch ubuntu-daily:focal net-snmp-bug1877027-focal
$ lxc shell net-snmp-bug1877027-focal
# apt update
# apt install -y snmpd snmp
# systemctl stop snmpd.service
# cat >> /var/lib/snmp/snmpd.conf << __EOF__
createUser testuser SHA "testpass" AES "testpass"
__EOF__
# cat >> /etc/snmp/snmpd.conf << __EOF__
rwuser testuser
__EOF__
# systemct start snmpd.service
# snmpbulkget -v3 -Cn1 -Cr1472 -l authPriv -u testuser -a SHA -A testpass -x AES -X testpass 127.0.0.1 1.3.6.1.2.1.1.5 1.3.6.1.2.1.1.7

You can check that snmpd crashed by doing:

# systemctl status snmpd.service

There is a PPA with the proposed fix here:

https://launchpad.net/~sergiodj/+archive/ubuntu/net-snmp-bug1877027

After installing it, one can check that the fix worked by doing:

# snmpbulkget -v3 -Cn1 -Cr1472 -l authPriv -u testuser -a SHA -A testpass -x AES -X testpass 127.0.0.1 1.3.6.1.2.1.1.5 1.3.6.1.2.1.1.7
Error in packet.
Reason: (genError) A general failure occured

Initially, I wasn't sure if this error should have happened or not. However, after compiling the upstream project and running it here, I noticed that it also displays this error message.

One of the persons interested in the bug was able to try the PPA and confirmed that it solves the segmentation fault for him.

autopkgtest is still happy:

autopkgtest [16:07:24]: test command1: perl -MSNMP -e1
autopkgtest [16:07:24]: test command1: [-----------------------
autopkgtest [16:07:24]: test command1: -----------------------]
autopkgtest [16:07:25]: test command1: - - - - - - - - - - results - - - - - - - - - -
command1 PASS
autopkgtest [16:07:26]: @@@@@@@@@@@@@@@@@@@@ summary
command1 PASS

To post a comment you must log in.
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Will look at this tomorrow.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Let us set this back to WIP (to clear the overview) until it is resolved either way

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

On Friday, July 03 2020, Christian Ehrhardt  wrote:

> Let us set this back to WIP (to clear the overview) until it is resolved either way

Thanks, Christian. This MP can be "abandoned" now, since the security
team has taken over and uploaded this fix on Focal. I can't change the
MP's status to anything other than "WIP"/"Needs Review"/"Merged", and I
think it's a good idea to keep this MP archived here (i.e., I don't want
to delete it). Could you please "abandon" it for me?

Thanks.

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

On Friday, July 03 2020, I wrote:

> On Friday, July 03 2020, Christian Ehrhardt  wrote:
>
>> Let us set this back to WIP (to clear the overview) until it is resolved either way
>
> Thanks, Christian. This MP can be "abandoned" now, since the security
> team has taken over and uploaded this fix on Focal. I can't change the
> MP's status to anything other than "WIP"/"Needs Review"/"Merged", and I
> think it's a good idea to keep this MP archived here (i.e., I don't want
> to delete it). Could you please "abandon" it for me?

FWIW, this is the upload done by the security team:

https://launchpad.net/ubuntu/+source/net-snmp/5.8+dfsg-2ubuntu2.1

Thanks,

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

Unmerged commits

5f5cf72... by Sergio Durigan Junior

changelog

7d6740d... by Sergio Durigan Junior

  * Fix segmentation fault that happens when using the snmpv3 protocol
    with snmpbulkget. (LP: #1877027)
    - d/p/move-securityStateRef-into-free_securityStateRef.patch:
      Consolidate the check of the securityStateRef pointer into the
      free_securityStateRef function.
    - d/p/prevent-snmpv3-bulkget-errors-double-free.patch:
      Prevent snmpv3 bulkget errors from becoming resulting in a
      double free.
    - d/p/fix-usmStateReference-free.patch:
      Fix typo on usm_free_usmStateReference from last patch.
    - d/p/unexport-struct-usmStateReference.patch:
      Unexport struct usmStateReference and to prevent ABI breakages,
      since it will be necessary to add a reference count to it.
    - d/p/introduce-refcount-usmStateReference.patch:
      Introduce refcount in the struct usmStateReference, and adjust
      code to properly use the field.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index d6d30a8..578573f 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,24 @@
1net-snmp (5.8+dfsg-2ubuntu2.1) focal; urgency=medium
2
3 * Fix segmentation fault that happens when using the snmpv3 protocol
4 with snmpbulkget. (LP: #1877027)
5 - d/p/move-securityStateRef-into-free_securityStateRef.patch:
6 Consolidate the check of the securityStateRef pointer into the
7 free_securityStateRef function.
8 - d/p/prevent-snmpv3-bulkget-errors-double-free.patch:
9 Prevent snmpv3 bulkget errors from becoming resulting in a
10 double free.
11 - d/p/fix-usmStateReference-free.patch:
12 Fix typo on usm_free_usmStateReference from last patch.
13 - d/p/unexport-struct-usmStateReference.patch:
14 Unexport struct usmStateReference and to prevent ABI breakages,
15 since it will be necessary to add a reference count to it.
16 - d/p/introduce-refcount-usmStateReference.patch:
17 Introduce refcount in the struct usmStateReference, and adjust
18 code to properly use the field.
19
20 -- Sergio Durigan Junior <sergio.durigan@canonical.com> Tue, 23 Jun 2020 14:57:12 -0400
21
1net-snmp (5.8+dfsg-2ubuntu2) focal; urgency=medium22net-snmp (5.8+dfsg-2ubuntu2) focal; urgency=medium
223
3 * d/p/lp1871307-log-once-proc-net-if_inet6-failure.patch (LP: #1871307):24 * d/p/lp1871307-log-once-proc-net-if_inet6-failure.patch (LP: #1871307):
diff --git a/debian/patches/fix-usmStateReference-free.patch b/debian/patches/fix-usmStateReference-free.patch
4new file mode 10064425new file mode 100644
index 0000000..0fe4602
--- /dev/null
+++ b/debian/patches/fix-usmStateReference-free.patch
@@ -0,0 +1,31 @@
1From: Ming Chen <ming0903@users.sourceforge.net>
2Date: Wed, 5 Jun 2019 19:58:44 -0700
3Subject: libsnmp: Fix usm_free_usmStateReference()
4
5See also https://sourceforge.net/p/net-snmp/bugs/2923/.
6
7Fixes: adc9b71aba91 ("snmpd: Avoid that snmpv3 bulkget errors result in a double free")
8
9Author: Ming Chen <ming0903@users.sourceforge.net>
10Origin: upstream, https://github.com/net-snmp/net-snmp/commit/7384a8b55
11Last-Update: 2020-06-17
12Bug: https://sourceforge.net/p/net-snmp/bugs/2923/
13Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
14Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
15---
16 snmplib/snmpusm.c | 2 +-
17 1 file changed, 1 insertion(+), 1 deletion(-)
18
19diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
20index c1a6dcc..0853e8d 100644
21--- a/snmplib/snmpusm.c
22+++ b/snmplib/snmpusm.c
23@@ -305,7 +305,7 @@ usm_free_usmStateReference(void *old)
24 SNMP_FREE(old_ref->usr_engine_id);
25 if (old_ref->usr_auth_protocol_length)
26 SNMP_FREE(old_ref->usr_auth_protocol);
27- if (old_ref->usr_auth_protocol_length)
28+ if (old_ref->usr_priv_protocol_length)
29 SNMP_FREE(old_ref->usr_priv_protocol);
30
31 if (old_ref->usr_auth_key_length && old_ref->usr_auth_key) {
diff --git a/debian/patches/introduce-refcount-usmStateReference.patch b/debian/patches/introduce-refcount-usmStateReference.patch
0new file mode 10064432new file mode 100644
index 0000000..8f6fb30
--- /dev/null
+++ b/debian/patches/introduce-refcount-usmStateReference.patch
@@ -0,0 +1,169 @@
1From: Bart Van Assche <bvanassche@acm.org>
2Date: Sat, 27 Jul 2019 19:34:09 -0700
3Subject: libsnmp,
4 USM: Introduce a reference count in struct usmStateReference
5
6This patch fixes https://sourceforge.net/p/net-snmp/bugs/2956/.
7
8Author: Bart Van Assche <bvanassche@acm.org>
9Origin: backport, https://github.com/net-snmp/net-snmp/commit/5f881d3bf
10Last-Update: 2020-06-17
11Bug: https://sourceforge.net/p/net-snmp/bugs/2956/
12Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
13Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
14---
15 snmplib/snmp_client.c | 21 ++++-----------
16 snmplib/snmpusm.c | 73 +++++++++++++++++++++++++++++++++------------------
17 2 files changed, 52 insertions(+), 42 deletions(-)
18
19diff --git a/snmplib/snmp_client.c b/snmplib/snmp_client.c
20index 2a46351..b2ea891 100644
21--- a/snmplib/snmp_client.c
22+++ b/snmplib/snmp_client.c
23@@ -402,27 +402,16 @@ _clone_pdu_header(netsnmp_pdu *pdu)
24 return NULL;
25 }
26
27- if (pdu->securityStateRef &&
28- pdu->command == SNMP_MSG_TRAP2) {
29-
30- ret = usm_clone_usmStateReference((struct usmStateReference *) pdu->securityStateRef,
31- (struct usmStateReference **) &newpdu->securityStateRef );
32-
33- if (ret)
34- {
35+ sptr = find_sec_mod(newpdu->securityModel);
36+ if (sptr && sptr->pdu_clone) {
37+ /* call security model if it needs to know about this */
38+ ret = sptr->pdu_clone(pdu, newpdu);
39+ if (ret) {
40 snmp_free_pdu(newpdu);
41 return NULL;
42 }
43 }
44
45- if ((sptr = find_sec_mod(newpdu->securityModel)) != NULL &&
46- sptr->pdu_clone != NULL) {
47- /*
48- * call security model if it needs to know about this
49- */
50- (*sptr->pdu_clone) (pdu, newpdu);
51- }
52-
53 return newpdu;
54 }
55
56diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
57index ec2074c..ba4ca5f 100644
58--- a/snmplib/snmpusm.c
59+++ b/snmplib/snmpusm.c
60@@ -85,6 +85,7 @@ netsnmp_feature_child_of(usm_support, usm_all)
61 netsnmp_feature_require(usm_support)
62
63 struct usmStateReference {
64+ int refcnt;
65 char *usr_name;
66 size_t usr_name_length;
67 u_char *usr_engine_id;
68@@ -301,43 +302,63 @@ free_enginetime_on_shutdown(int majorid, int minorid, void *serverarg,
69 struct usmStateReference *
70 usm_malloc_usmStateReference(void)
71 {
72- struct usmStateReference *retval = (struct usmStateReference *)
73- calloc(1, sizeof(struct usmStateReference));
74+ struct usmStateReference *retval;
75+
76+ retval = calloc(1, sizeof(struct usmStateReference));
77+ if (retval)
78+ retval->refcnt = 1;
79
80 return retval;
81 } /* end usm_malloc_usmStateReference() */
82
83+static int
84+usm_clone(netsnmp_pdu *pdu, netsnmp_pdu *new_pdu)
85+{
86+ struct usmStateReference *ref = pdu->securityStateRef;
87+ struct usmStateReference **new_ref =
88+ (struct usmStateReference **)&new_pdu->securityStateRef;
89+ int ret = 0;
90+
91+ if (!ref)
92+ return ret;
93+
94+ if (pdu->command == SNMP_MSG_TRAP2) {
95+ netsnmp_assert(pdu->securityModel == SNMP_DEFAULT_SECMODEL);
96+ ret = usm_clone_usmStateReference(ref, new_ref);
97+ } else {
98+ netsnmp_assert(ref == *new_ref);
99+ ref->refcnt++;
100+ }
101+
102+ return ret;
103+}
104
105 void
106 usm_free_usmStateReference(void *old)
107 {
108- struct usmStateReference *old_ref = (struct usmStateReference *) old;
109-
110- if (old_ref) {
111-
112- if (old_ref->usr_name_length)
113- SNMP_FREE(old_ref->usr_name);
114- if (old_ref->usr_engine_id_length)
115- SNMP_FREE(old_ref->usr_engine_id);
116- if (old_ref->usr_auth_protocol_length)
117- SNMP_FREE(old_ref->usr_auth_protocol);
118- if (old_ref->usr_priv_protocol_length)
119- SNMP_FREE(old_ref->usr_priv_protocol);
120-
121- if (old_ref->usr_auth_key_length && old_ref->usr_auth_key) {
122- SNMP_ZERO(old_ref->usr_auth_key, old_ref->usr_auth_key_length);
123- SNMP_FREE(old_ref->usr_auth_key);
124- }
125- if (old_ref->usr_priv_key_length && old_ref->usr_priv_key) {
126- SNMP_ZERO(old_ref->usr_priv_key, old_ref->usr_priv_key_length);
127- SNMP_FREE(old_ref->usr_priv_key);
128- }
129+ struct usmStateReference *ref = old;
130+
131+ if (!ref)
132+ return;
133
134- SNMP_ZERO(old_ref, sizeof(*old_ref));
135- SNMP_FREE(old_ref);
136+ if (--ref->refcnt > 0)
137+ return;
138+
139+ SNMP_FREE(ref->usr_name);
140+ SNMP_FREE(ref->usr_engine_id);
141+ SNMP_FREE(ref->usr_auth_protocol);
142+ SNMP_FREE(ref->usr_priv_protocol);
143
144+ if (ref->usr_auth_key_length && ref->usr_auth_key) {
145+ SNMP_ZERO(ref->usr_auth_key, ref->usr_auth_key_length);
146+ SNMP_FREE(ref->usr_auth_key);
147+ }
148+ if (ref->usr_priv_key_length && ref->usr_priv_key) {
149+ SNMP_ZERO(ref->usr_priv_key, ref->usr_priv_key_length);
150+ SNMP_FREE(ref->usr_priv_key);
151 }
152
153+ SNMP_FREE(ref);
154 } /* end usm_free_usmStateReference() */
155
156 struct usmUser *
157@@ -3337,6 +3358,7 @@ init_usm(void)
158 def->encode_reverse = usm_secmod_rgenerate_out_msg;
159 def->encode_forward = usm_secmod_generate_out_msg;
160 def->decode = usm_secmod_process_in_msg;
161+ def->pdu_clone = usm_clone;
162 def->pdu_free_state_ref = usm_free_usmStateReference;
163 def->session_setup = usm_session_init;
164 def->handle_report = usm_handle_report;
165@@ -5110,4 +5132,3 @@ get_default_privtype(size_t * len)
166 *len = defaultPrivTypeLen;
167 return defaultPrivType;
168 }
169-
diff --git a/debian/patches/move-free-securityStateRef-into-snmp-free-pdu.patch b/debian/patches/move-free-securityStateRef-into-snmp-free-pdu.patch
0new file mode 100644170new file mode 100644
index 0000000..a568095
--- /dev/null
+++ b/debian/patches/move-free-securityStateRef-into-snmp-free-pdu.patch
@@ -0,0 +1,92 @@
1From: Bart Van Assche <bvanassche@acm.org>
2Date: Tue, 13 Aug 2019 20:54:23 -0700
3Subject: libsnmp: Move the free_securityStateRef() call into snmp_free_pdu()
4
5This patch fixes a memory leak that was introduced in commit 5f881d3bf245
6("libsnmp, USM: Introduce a reference count in struct usmStateReference").
7
8This patch partially reverts commit adc9b71aba91 ("snmpd: Avoid that snmpv3
9bulkget errors result in a double free").
10
11See also https://sourceforge.net/p/net-snmp/bugs/2938/.
12
13Author: Bart Van Assche <bvanassche@acm.org>
14Origin: backport, https://github.com/net-snmp/net-snmp/commit/87bd90d04
15Last-Update: 2020-06-17
16Bug: https://sourceforge.net/p/net-snmp/bugs/2938/
17Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
18Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
19---
20 include/net-snmp/pdu_api.h | 2 --
21 snmplib/snmp_api.c | 24 +++---------------------
22 2 files changed, 3 insertions(+), 23 deletions(-)
23
24diff --git a/include/net-snmp/pdu_api.h b/include/net-snmp/pdu_api.h
25index 270aff0..125595d 100644
26--- a/include/net-snmp/pdu_api.h
27+++ b/include/net-snmp/pdu_api.h
28@@ -19,8 +19,6 @@ NETSNMP_IMPORT
29 netsnmp_pdu *snmp_fix_pdu( netsnmp_pdu *pdu, int idx);
30 NETSNMP_IMPORT
31 void snmp_free_pdu( netsnmp_pdu *pdu);
32-NETSNMP_IMPORT
33-void snmp_free_securityStateRef( netsnmp_pdu *pdu);
34
35 #ifdef __cplusplus
36 }
37diff --git a/snmplib/snmp_api.c b/snmplib/snmp_api.c
38index 046b247..0fea19e 100644
39--- a/snmplib/snmp_api.c
40+++ b/snmplib/snmp_api.c
41@@ -4033,17 +4033,6 @@ free_securityStateRef(netsnmp_pdu* pdu)
42 pdu->securityStateRef = NULL;
43 }
44
45-/*
46- * This function is here to provide a separate call to
47- * free the securityStateRef memory. This is needed to prevent
48- * a double free if this memory is freed in snmp_free_pdu.
49- */
50-void
51-snmp_free_securityStateRef(netsnmp_pdu* pdu)
52-{
53- free_securityStateRef(pdu);
54-}
55-
56 #define ERROR_STAT_LENGTH 11
57
58 int
59@@ -5493,6 +5482,9 @@ snmp_free_pdu(netsnmp_pdu *pdu)
60 return;
61 }
62 */
63+
64+ free_securityStateRef(pdu);
65+
66 if ((sptr = find_sec_mod(pdu->securityModel)) != NULL &&
67 sptr->pdu_free != NULL) {
68 (*sptr->pdu_free) (pdu);
69@@ -5647,10 +5639,6 @@ _sess_process_packet_parse_pdu(void *sessp, netsnmp_session * sp,
70 }
71
72 if (ret != SNMP_ERR_NOERROR) {
73- /*
74- * Call the security model to free any securityStateRef supplied w/ msg.
75- */
76- free_securityStateRef(pdu);
77 snmp_free_pdu(pdu);
78 return NULL;
79 }
80@@ -5822,12 +5810,6 @@ _sess_process_packet_handle_pdu(void *sessp, netsnmp_session * sp,
81 }
82 }
83
84- /*
85- * Call USM to free any securityStateRef supplied with the message.
86- */
87- if (pdu->command == SNMP_MSG_TRAP2)
88- free_securityStateRef(pdu);
89-
90 if (!handled) {
91 if (sp->flags & SNMP_FLAGS_SHARED_SOCKET)
92 return -2;
diff --git a/debian/patches/move-securityStateRef-into-free_securityStateRef.patch b/debian/patches/move-securityStateRef-into-free_securityStateRef.patch
0new file mode 10064493new file mode 100644
index 0000000..fe6697c
--- /dev/null
+++ b/debian/patches/move-securityStateRef-into-free_securityStateRef.patch
@@ -0,0 +1,78 @@
1From: Bart Van Assche <bvanassche@acm.org>
2Date: Sat, 25 May 2019 16:33:31 +0200
3Subject: libsnmp: Move the securityStateRef check into
4 free_securityStateRef()
5
6Instead of making each free_securityStateRef() caller check the
7securityStateRef pointer, move that check into free_securityStateRef().
8
9Author: Bart Van Assche <bvanassche@acm.org>
10Origin: backport, https://github.com/net-snmp/net-snmp/commit/92ccd5a82
11Last-Update: 2020-06-17
12Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
13Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
14---
15 agent/snmp_agent.c | 1 +
16 snmplib/snmp_api.c | 21 ++++++++++-----------
17 2 files changed, 11 insertions(+), 11 deletions(-)
18
19diff --git a/snmplib/snmp_api.c b/snmplib/snmp_api.c
20index 6754fa6..bfc052b 100644
21--- a/snmplib/snmp_api.c
22+++ b/snmplib/snmp_api.c
23@@ -4011,7 +4011,12 @@ snmpv3_parse(netsnmp_pdu *pdu,
24 static void
25 free_securityStateRef(netsnmp_pdu* pdu)
26 {
27- struct snmp_secmod_def *sptr = find_sec_mod(pdu->securityModel);
28+ struct snmp_secmod_def *sptr;
29+
30+ if (!pdu->securityStateRef)
31+ return;
32+
33+ sptr = find_sec_mod(pdu->securityModel);
34 if (sptr) {
35 if (sptr->pdu_free_state_ref) {
36 (*sptr->pdu_free_state_ref) (pdu->securityStateRef);
37@@ -4122,9 +4127,7 @@ snmpv3_make_report(netsnmp_pdu *pdu, int error)
38 * FIX - yes they should but USM needs to follow new EoP to determine
39 * which cached values to use
40 */
41- if (pdu->securityStateRef) {
42- free_securityStateRef(pdu);
43- }
44+ free_securityStateRef(pdu);
45
46 if (error == SNMPERR_USM_NOTINTIMEWINDOW) {
47 pdu->securityLevel = SNMP_SEC_LEVEL_AUTHNOPRIV;
48@@ -5636,9 +5639,7 @@ _sess_process_packet_parse_pdu(void *sessp, netsnmp_session * sp,
49 /*
50 * Call the security model to free any securityStateRef supplied w/ msg.
51 */
52- if (pdu->securityStateRef != NULL) {
53- free_securityStateRef(pdu);
54- }
55+ free_securityStateRef(pdu);
56 snmp_free_pdu(pdu);
57 return NULL;
58 }
59@@ -5662,9 +5663,7 @@ _sess_process_packet_handle_pdu(void *sessp, netsnmp_session * sp,
60 /*
61 * Call USM to free any securityStateRef supplied with the message.
62 */
63- if (pdu->securityStateRef) {
64- free_securityStateRef(pdu);
65- }
66+ free_securityStateRef(pdu);
67
68 for (rp = isp->requests; rp; orp = rp, rp = rp->next_request) {
69 snmp_callback callback;
70@@ -5815,7 +5814,7 @@ _sess_process_packet_handle_pdu(void *sessp, netsnmp_session * sp,
71 /*
72 * Call USM to free any securityStateRef supplied with the message.
73 */
74- if (pdu->securityStateRef && pdu->command == SNMP_MSG_TRAP2)
75+ if (pdu->command == SNMP_MSG_TRAP2)
76 free_securityStateRef(pdu);
77
78 if (!handled) {
diff --git a/debian/patches/prevent-snmpv3-bulkget-errors-double-free.patch b/debian/patches/prevent-snmpv3-bulkget-errors-double-free.patch
0new file mode 10064479new file mode 100644
index 0000000..61b8348
--- /dev/null
+++ b/debian/patches/prevent-snmpv3-bulkget-errors-double-free.patch
@@ -0,0 +1,349 @@
1From: Sam Tannous <stannous@cumulusnetworks.com>
2Date: Wed, 10 Apr 2019 06:57:21 -0700
3Subject: snmpd: Avoid that snmpv3 bulkget errors result in a double free
4
5See also https://sourceforge.net/p/net-snmp/bugs/2923/.
6See also https://sourceforge.net/p/net-snmp/patches/1388/.
7
8Author: Ming Chen <ming0903@users.sourceforge.net>
9Origin: upstream, https://github.com/net-snmp/net-snmp/commit/adc9b71ab
10Last-Update: 2020-06-17
11Bug: https://sourceforge.net/p/net-snmp/bugs/2923/
12Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
13Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
14---
15 include/net-snmp/pdu_api.h | 2 ++
16 snmplib/snmp_api.c | 11 ++++++++++
17 snmplib/snmpusm.c | 51 ++++++++++------------------------------------
18 3 files changed, 24 insertions(+), 40 deletions(-)
19
20diff --git a/include/net-snmp/pdu_api.h b/include/net-snmp/pdu_api.h
21index 125595d..270aff0 100644
22--- a/include/net-snmp/pdu_api.h
23+++ b/include/net-snmp/pdu_api.h
24@@ -19,6 +19,8 @@ NETSNMP_IMPORT
25 netsnmp_pdu *snmp_fix_pdu( netsnmp_pdu *pdu, int idx);
26 NETSNMP_IMPORT
27 void snmp_free_pdu( netsnmp_pdu *pdu);
28+NETSNMP_IMPORT
29+void snmp_free_securityStateRef( netsnmp_pdu *pdu);
30
31 #ifdef __cplusplus
32 }
33diff --git a/snmplib/snmp_api.c b/snmplib/snmp_api.c
34index bfc052b..046b247 100644
35--- a/snmplib/snmp_api.c
36+++ b/snmplib/snmp_api.c
37@@ -4033,6 +4033,17 @@ free_securityStateRef(netsnmp_pdu* pdu)
38 pdu->securityStateRef = NULL;
39 }
40
41+/*
42+ * This function is here to provide a separate call to
43+ * free the securityStateRef memory. This is needed to prevent
44+ * a double free if this memory is freed in snmp_free_pdu.
45+ */
46+void
47+snmp_free_securityStateRef(netsnmp_pdu* pdu)
48+{
49+ free_securityStateRef(pdu);
50+}
51+
52 #define ERROR_STAT_LENGTH 11
53
54 int
55diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
56index e697c23..c1a6dcc 100644
57--- a/snmplib/snmpusm.c
58+++ b/snmplib/snmpusm.c
59@@ -299,16 +299,20 @@ usm_free_usmStateReference(void *old)
60
61 if (old_ref) {
62
63- SNMP_FREE(old_ref->usr_name);
64- SNMP_FREE(old_ref->usr_engine_id);
65- SNMP_FREE(old_ref->usr_auth_protocol);
66- SNMP_FREE(old_ref->usr_priv_protocol);
67-
68- if (old_ref->usr_auth_key) {
69+ if (old_ref->usr_name_length)
70+ SNMP_FREE(old_ref->usr_name);
71+ if (old_ref->usr_engine_id_length)
72+ SNMP_FREE(old_ref->usr_engine_id);
73+ if (old_ref->usr_auth_protocol_length)
74+ SNMP_FREE(old_ref->usr_auth_protocol);
75+ if (old_ref->usr_auth_protocol_length)
76+ SNMP_FREE(old_ref->usr_priv_protocol);
77+
78+ if (old_ref->usr_auth_key_length && old_ref->usr_auth_key) {
79 SNMP_ZERO(old_ref->usr_auth_key, old_ref->usr_auth_key_length);
80 SNMP_FREE(old_ref->usr_auth_key);
81 }
82- if (old_ref->usr_priv_key) {
83+ if (old_ref->usr_priv_key_length && old_ref->usr_priv_key) {
84 SNMP_ZERO(old_ref->usr_priv_key, old_ref->usr_priv_key_length);
85 SNMP_FREE(old_ref->usr_priv_key);
86 }
87@@ -1039,7 +1043,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
88 if ((user = usm_get_user(secEngineID, secEngineIDLen, secName))
89 == NULL && secLevel != SNMP_SEC_LEVEL_NOAUTH) {
90 DEBUGMSGTL(("usm", "Unknown User(%s)\n", secName));
91- usm_free_usmStateReference(secStateRef);
92 return SNMPERR_USM_UNKNOWNSECURITYNAME;
93 }
94
95@@ -1091,7 +1094,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
96 thePrivProtocolLength) == 1) {
97 DEBUGMSGTL(("usm", "Unsupported Security Level (%d)\n",
98 theSecLevel));
99- usm_free_usmStateReference(secStateRef);
100 return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL;
101 }
102
103@@ -1121,7 +1123,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
104 &msgAuthParmLen, &msgPrivParmLen, &otstlen,
105 &seq_len, &msgSecParmLen) == -1) {
106 DEBUGMSGTL(("usm", "Failed calculating offsets.\n"));
107- usm_free_usmStateReference(secStateRef);
108 return SNMPERR_USM_GENERICERROR;
109 }
110
111@@ -1143,7 +1144,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
112 ptr = *wholeMsg = globalData;
113 if (theTotalLength > *wholeMsgLen) {
114 DEBUGMSGTL(("usm", "Message won't fit in buffer.\n"));
115- usm_free_usmStateReference(secStateRef);
116 return SNMPERR_USM_GENERICERROR;
117 }
118
119@@ -1169,7 +1169,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
120 htonl(boots_uint), htonl(time_uint),
121 &ptr[privParamsOffset]) == -1) {
122 DEBUGMSGTL(("usm", "Can't set AES iv.\n"));
123- usm_free_usmStateReference(secStateRef);
124 return SNMPERR_USM_GENERICERROR;
125 }
126 }
127@@ -1185,7 +1184,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
128 &ptr[privParamsOffset])
129 == -1)) {
130 DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n"));
131- usm_free_usmStateReference(secStateRef);
132 return SNMPERR_USM_GENERICERROR;
133 }
134 }
135@@ -1198,7 +1196,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
136 &ptr[dataOffset], &encrypted_length)
137 != SNMP_ERR_NOERROR) {
138 DEBUGMSGTL(("usm", "encryption error.\n"));
139- usm_free_usmStateReference(secStateRef);
140 return SNMPERR_USM_ENCRYPTIONERROR;
141 }
142 #ifdef NETSNMP_ENABLE_TESTING_CODE
143@@ -1226,7 +1223,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
144 if ((encrypted_length != (theTotalLength - dataOffset))
145 || (salt_length != msgPrivParmLen)) {
146 DEBUGMSGTL(("usm", "encryption length error.\n"));
147- usm_free_usmStateReference(secStateRef);
148 return SNMPERR_USM_ENCRYPTIONERROR;
149 }
150
151@@ -1362,7 +1358,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
152
153 if (temp_sig == NULL) {
154 DEBUGMSGTL(("usm", "Out of memory.\n"));
155- usm_free_usmStateReference(secStateRef);
156 return SNMPERR_USM_GENERICERROR;
157 }
158
159@@ -1376,7 +1371,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
160 SNMP_ZERO(temp_sig, temp_sig_len);
161 SNMP_FREE(temp_sig);
162 DEBUGMSGTL(("usm", "Signing failed.\n"));
163- usm_free_usmStateReference(secStateRef);
164 return SNMPERR_USM_AUTHENTICATIONFAILURE;
165 }
166
167@@ -1384,7 +1378,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
168 SNMP_ZERO(temp_sig, temp_sig_len);
169 SNMP_FREE(temp_sig);
170 DEBUGMSGTL(("usm", "Signing lengths failed.\n"));
171- usm_free_usmStateReference(secStateRef);
172 return SNMPERR_USM_AUTHENTICATIONFAILURE;
173 }
174
175@@ -1398,7 +1391,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
176 /*
177 * endif -- create keyed hash
178 */
179- usm_free_usmStateReference(secStateRef);
180
181 DEBUGMSGTL(("usm", "USM processing completed.\n"));
182
183@@ -1548,7 +1540,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
184 if ((user = usm_get_user(secEngineID, secEngineIDLen, secName))
185 == NULL && secLevel != SNMP_SEC_LEVEL_NOAUTH) {
186 DEBUGMSGTL(("usm", "Unknown User\n"));
187- usm_free_usmStateReference(secStateRef);
188 return SNMPERR_USM_UNKNOWNSECURITYNAME;
189 }
190
191@@ -1601,7 +1592,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
192 DEBUGMSGTL(("usm", "Unsupported Security Level or type (%d)\n",
193 theSecLevel));
194
195- usm_free_usmStateReference(secStateRef);
196 return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL;
197 }
198
199@@ -1636,7 +1626,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
200 DEBUGMSGTL(("usm",
201 "couldn't malloc %d bytes for encrypted PDU\n",
202 (int)ciphertextlen));
203- usm_free_usmStateReference(secStateRef);
204 return SNMPERR_MALLOC;
205 }
206
207@@ -1652,7 +1641,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
208 htonl(boots_uint), htonl(time_uint),
209 iv) == -1) {
210 DEBUGMSGTL(("usm", "Can't set AES iv.\n"));
211- usm_free_usmStateReference(secStateRef);
212 SNMP_FREE(ciphertext);
213 return SNMPERR_USM_GENERICERROR;
214 }
215@@ -1667,7 +1655,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
216 thePrivKeyLength - 8,
217 iv) == -1)) {
218 DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n"));
219- usm_free_usmStateReference(secStateRef);
220 SNMP_FREE(ciphertext);
221 return SNMPERR_USM_GENERICERROR;
222 }
223@@ -1686,7 +1673,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
224 scopedPdu, scopedPduLen,
225 ciphertext, &ciphertextlen) != SNMP_ERR_NOERROR) {
226 DEBUGMSGTL(("usm", "encryption error.\n"));
227- usm_free_usmStateReference(secStateRef);
228 SNMP_FREE(ciphertext);
229 return SNMPERR_USM_ENCRYPTIONERROR;
230 }
231@@ -1703,7 +1689,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
232 ciphertext, ciphertextlen);
233 if (rc == 0) {
234 DEBUGMSGTL(("usm", "Encryption failed.\n"));
235- usm_free_usmStateReference(secStateRef);
236 SNMP_FREE(ciphertext);
237 return SNMPERR_USM_ENCRYPTIONERROR;
238 }
239@@ -1743,7 +1728,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
240 DEBUGINDENTLESS();
241 if (rc == 0) {
242 DEBUGMSGTL(("usm", "building privParams failed.\n"));
243- usm_free_usmStateReference(secStateRef);
244 return SNMPERR_TOO_LONG;
245 }
246
247@@ -1766,7 +1750,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
248 DEBUGINDENTLESS();
249 if (rc == 0) {
250 DEBUGMSGTL(("usm", "building authParams failed.\n"));
251- usm_free_usmStateReference(secStateRef);
252 return SNMPERR_TOO_LONG;
253 }
254
255@@ -1789,7 +1772,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
256 DEBUGINDENTLESS();
257 if (rc == 0) {
258 DEBUGMSGTL(("usm", "building authParams failed.\n"));
259- usm_free_usmStateReference(secStateRef);
260 return SNMPERR_TOO_LONG;
261 }
262
263@@ -1805,7 +1787,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
264 if (rc == 0) {
265 DEBUGMSGTL(("usm",
266 "building msgAuthoritativeEngineTime failed.\n"));
267- usm_free_usmStateReference(secStateRef);
268 return SNMPERR_TOO_LONG;
269 }
270
271@@ -1821,7 +1802,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
272 if (rc == 0) {
273 DEBUGMSGTL(("usm",
274 "building msgAuthoritativeEngineBoots failed.\n"));
275- usm_free_usmStateReference(secStateRef);
276 return SNMPERR_TOO_LONG;
277 }
278
279@@ -1833,7 +1813,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
280 DEBUGINDENTLESS();
281 if (rc == 0) {
282 DEBUGMSGTL(("usm", "building msgAuthoritativeEngineID failed.\n"));
283- usm_free_usmStateReference(secStateRef);
284 return SNMPERR_TOO_LONG;
285 }
286
287@@ -1846,7 +1825,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
288 *offset - sp_offset);
289 if (rc == 0) {
290 DEBUGMSGTL(("usm", "building usm security parameters failed.\n"));
291- usm_free_usmStateReference(secStateRef);
292 return SNMPERR_TOO_LONG;
293 }
294
295@@ -1860,7 +1838,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
296
297 if (rc == 0) {
298 DEBUGMSGTL(("usm", "building msgSecurityParameters failed.\n"));
299- usm_free_usmStateReference(secStateRef);
300 return SNMPERR_TOO_LONG;
301 }
302
303@@ -1870,7 +1847,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
304 while ((*wholeMsgLen - *offset) < globalDataLen) {
305 if (!asn_realloc(wholeMsg, wholeMsgLen)) {
306 DEBUGMSGTL(("usm", "building global data failed.\n"));
307- usm_free_usmStateReference(secStateRef);
308 return SNMPERR_TOO_LONG;
309 }
310 }
311@@ -1886,7 +1862,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
312 ASN_CONSTRUCTOR), *offset);
313 if (rc == 0) {
314 DEBUGMSGTL(("usm", "building master packet sequence failed.\n"));
315- usm_free_usmStateReference(secStateRef);
316 return SNMPERR_TOO_LONG;
317 }
318
319@@ -1904,7 +1879,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
320
321 if (temp_sig == NULL) {
322 DEBUGMSGTL(("usm", "Out of memory.\n"));
323- usm_free_usmStateReference(secStateRef);
324 return SNMPERR_USM_GENERICERROR;
325 }
326
327@@ -1915,14 +1889,12 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
328 != SNMP_ERR_NOERROR) {
329 SNMP_FREE(temp_sig);
330 DEBUGMSGTL(("usm", "Signing failed.\n"));
331- usm_free_usmStateReference(secStateRef);
332 return SNMPERR_USM_AUTHENTICATIONFAILURE;
333 }
334
335 if (temp_sig_len != msgAuthParmLen) {
336 SNMP_FREE(temp_sig);
337 DEBUGMSGTL(("usm", "Signing lengths failed.\n"));
338- usm_free_usmStateReference(secStateRef);
339 return SNMPERR_USM_AUTHENTICATIONFAILURE;
340 }
341
342@@ -1933,7 +1905,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
343 /*
344 * endif -- create keyed hash
345 */
346- usm_free_usmStateReference(secStateRef);
347 DEBUGMSGTL(("usm", "USM processing completed.\n"));
348 return SNMPERR_SUCCESS;
349 } /* end usm_rgenerate_out_msg() */
diff --git a/debian/patches/series b/debian/patches/series
index 8160545..eac6e7a 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -37,3 +37,9 @@ autofs-skip-autofs-entries.patch
37autofs-fix-a-recently-introduced-bug.patch37autofs-fix-a-recently-introduced-bug.patch
38fix-check-hr-filesys-autofs.patch38fix-check-hr-filesys-autofs.patch
39lp1871307-log-once-proc-net-if_inet6-failure.patch39lp1871307-log-once-proc-net-if_inet6-failure.patch
40move-securityStateRef-into-free_securityStateRef.patch
41prevent-snmpv3-bulkget-errors-double-free.patch
42fix-usmStateReference-free.patch
43unexport-struct-usmStateReference.patch
44introduce-refcount-usmStateReference.patch
45move-free-securityStateRef-into-snmp-free-pdu.patch
diff --git a/debian/patches/unexport-struct-usmStateReference.patch b/debian/patches/unexport-struct-usmStateReference.patch
40new file mode 10064446new file mode 100644
index 0000000..aeb3081
--- /dev/null
+++ b/debian/patches/unexport-struct-usmStateReference.patch
@@ -0,0 +1,74 @@
1From: Bart Van Assche <bvanassche@acm.org>
2Date: Tue, 23 Jul 2019 10:52:28 -0700
3Subject: libsnmp: Unexport struct usmStateReference
4
5Certain snmpd crashes can only be fixed by introducing a reference
6count in struct usmStateReference. Unexport that structure such that
7changing it does not affect the ABI.
8
9Author: Bart Van Assche <bvanassche@acm.org>
10Origin: upstream, https://github.com/net-snmp/net-snmp/commit/39381c4d2
11Last-Update: 2020-06-17
12Bug: https://sourceforge.net/p/net-snmp/bugs/2956/
13Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
14Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
15---
16 include/net-snmp/library/snmpusm.h | 17 +----------------
17 snmplib/snmpusm.c | 16 ++++++++++++++++
18 2 files changed, 17 insertions(+), 16 deletions(-)
19
20diff --git a/include/net-snmp/library/snmpusm.h b/include/net-snmp/library/snmpusm.h
21index 3f60787..49061d8 100644
22--- a/include/net-snmp/library/snmpusm.h
23+++ b/include/net-snmp/library/snmpusm.h
24@@ -42,22 +42,7 @@ extern "C" {
25 /*
26 * Structures.
27 */
28- struct usmStateReference {
29- char *usr_name;
30- size_t usr_name_length;
31- u_char *usr_engine_id;
32- size_t usr_engine_id_length;
33- oid *usr_auth_protocol;
34- size_t usr_auth_protocol_length;
35- u_char *usr_auth_key;
36- size_t usr_auth_key_length;
37- oid *usr_priv_protocol;
38- size_t usr_priv_protocol_length;
39- u_char *usr_priv_key;
40- size_t usr_priv_key_length;
41- u_int usr_sec_level;
42- };
43-
44+ struct usmStateReference;
45
46 /*
47 * struct usmUser: a structure to represent a given user in a list
48diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
49index 0853e8d..ec2074c 100644
50--- a/snmplib/snmpusm.c
51+++ b/snmplib/snmpusm.c
52@@ -84,6 +84,22 @@ netsnmp_feature_child_of(usm_support, usm_all)
53
54 netsnmp_feature_require(usm_support)
55
56+struct usmStateReference {
57+ char *usr_name;
58+ size_t usr_name_length;
59+ u_char *usr_engine_id;
60+ size_t usr_engine_id_length;
61+ oid *usr_auth_protocol;
62+ size_t usr_auth_protocol_length;
63+ u_char *usr_auth_key;
64+ size_t usr_auth_key_length;
65+ oid *usr_priv_protocol;
66+ size_t usr_priv_protocol_length;
67+ u_char *usr_priv_key;
68+ size_t usr_priv_key_length;
69+ u_int usr_sec_level;
70+};
71+
72 oid usmNoAuthProtocol[10] = { NETSNMP_USMAUTH_BASE_OID,
73 NETSNMP_USMAUTH_NOAUTH };
74 #ifndef NETSNMP_DISABLE_MD5

Subscribers

People subscribed via source and target branches