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

Proposed by Sergio Durigan Junior on 2020-06-23
Status: Rejected
Rejected by: Andreas Hasenack on 2020-07-03
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 2020-06-23 Pending
Canonical Server Team 2020-06-23 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.
Andreas Hasenack (ahasenack) wrote :

Will look at this tomorrow.

Christian Ehrhardt  (paelzer) wrote :

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

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

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 on 2020-06-23

changelog

7d6740d... by Sergio Durigan Junior on 2020-06-19

  * 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
1diff --git a/debian/changelog b/debian/changelog
2index d6d30a8..578573f 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,24 @@
6+net-snmp (5.8+dfsg-2ubuntu2.1) focal; urgency=medium
7+
8+ * Fix segmentation fault that happens when using the snmpv3 protocol
9+ with snmpbulkget. (LP: #1877027)
10+ - d/p/move-securityStateRef-into-free_securityStateRef.patch:
11+ Consolidate the check of the securityStateRef pointer into the
12+ free_securityStateRef function.
13+ - d/p/prevent-snmpv3-bulkget-errors-double-free.patch:
14+ Prevent snmpv3 bulkget errors from becoming resulting in a
15+ double free.
16+ - d/p/fix-usmStateReference-free.patch:
17+ Fix typo on usm_free_usmStateReference from last patch.
18+ - d/p/unexport-struct-usmStateReference.patch:
19+ Unexport struct usmStateReference and to prevent ABI breakages,
20+ since it will be necessary to add a reference count to it.
21+ - d/p/introduce-refcount-usmStateReference.patch:
22+ Introduce refcount in the struct usmStateReference, and adjust
23+ code to properly use the field.
24+
25+ -- Sergio Durigan Junior <sergio.durigan@canonical.com> Tue, 23 Jun 2020 14:57:12 -0400
26+
27 net-snmp (5.8+dfsg-2ubuntu2) focal; urgency=medium
28
29 * d/p/lp1871307-log-once-proc-net-if_inet6-failure.patch (LP: #1871307):
30diff --git a/debian/patches/fix-usmStateReference-free.patch b/debian/patches/fix-usmStateReference-free.patch
31new file mode 100644
32index 0000000..0fe4602
33--- /dev/null
34+++ b/debian/patches/fix-usmStateReference-free.patch
35@@ -0,0 +1,31 @@
36+From: Ming Chen <ming0903@users.sourceforge.net>
37+Date: Wed, 5 Jun 2019 19:58:44 -0700
38+Subject: libsnmp: Fix usm_free_usmStateReference()
39+
40+See also https://sourceforge.net/p/net-snmp/bugs/2923/.
41+
42+Fixes: adc9b71aba91 ("snmpd: Avoid that snmpv3 bulkget errors result in a double free")
43+
44+Author: Ming Chen <ming0903@users.sourceforge.net>
45+Origin: upstream, https://github.com/net-snmp/net-snmp/commit/7384a8b55
46+Last-Update: 2020-06-17
47+Bug: https://sourceforge.net/p/net-snmp/bugs/2923/
48+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
49+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
50+---
51+ snmplib/snmpusm.c | 2 +-
52+ 1 file changed, 1 insertion(+), 1 deletion(-)
53+
54+diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
55+index c1a6dcc..0853e8d 100644
56+--- a/snmplib/snmpusm.c
57++++ b/snmplib/snmpusm.c
58+@@ -305,7 +305,7 @@ usm_free_usmStateReference(void *old)
59+ SNMP_FREE(old_ref->usr_engine_id);
60+ if (old_ref->usr_auth_protocol_length)
61+ SNMP_FREE(old_ref->usr_auth_protocol);
62+- if (old_ref->usr_auth_protocol_length)
63++ if (old_ref->usr_priv_protocol_length)
64+ SNMP_FREE(old_ref->usr_priv_protocol);
65+
66+ if (old_ref->usr_auth_key_length && old_ref->usr_auth_key) {
67diff --git a/debian/patches/introduce-refcount-usmStateReference.patch b/debian/patches/introduce-refcount-usmStateReference.patch
68new file mode 100644
69index 0000000..8f6fb30
70--- /dev/null
71+++ b/debian/patches/introduce-refcount-usmStateReference.patch
72@@ -0,0 +1,169 @@
73+From: Bart Van Assche <bvanassche@acm.org>
74+Date: Sat, 27 Jul 2019 19:34:09 -0700
75+Subject: libsnmp,
76+ USM: Introduce a reference count in struct usmStateReference
77+
78+This patch fixes https://sourceforge.net/p/net-snmp/bugs/2956/.
79+
80+Author: Bart Van Assche <bvanassche@acm.org>
81+Origin: backport, https://github.com/net-snmp/net-snmp/commit/5f881d3bf
82+Last-Update: 2020-06-17
83+Bug: https://sourceforge.net/p/net-snmp/bugs/2956/
84+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
85+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
86+---
87+ snmplib/snmp_client.c | 21 ++++-----------
88+ snmplib/snmpusm.c | 73 +++++++++++++++++++++++++++++++++------------------
89+ 2 files changed, 52 insertions(+), 42 deletions(-)
90+
91+diff --git a/snmplib/snmp_client.c b/snmplib/snmp_client.c
92+index 2a46351..b2ea891 100644
93+--- a/snmplib/snmp_client.c
94++++ b/snmplib/snmp_client.c
95+@@ -402,27 +402,16 @@ _clone_pdu_header(netsnmp_pdu *pdu)
96+ return NULL;
97+ }
98+
99+- if (pdu->securityStateRef &&
100+- pdu->command == SNMP_MSG_TRAP2) {
101+-
102+- ret = usm_clone_usmStateReference((struct usmStateReference *) pdu->securityStateRef,
103+- (struct usmStateReference **) &newpdu->securityStateRef );
104+-
105+- if (ret)
106+- {
107++ sptr = find_sec_mod(newpdu->securityModel);
108++ if (sptr && sptr->pdu_clone) {
109++ /* call security model if it needs to know about this */
110++ ret = sptr->pdu_clone(pdu, newpdu);
111++ if (ret) {
112+ snmp_free_pdu(newpdu);
113+ return NULL;
114+ }
115+ }
116+
117+- if ((sptr = find_sec_mod(newpdu->securityModel)) != NULL &&
118+- sptr->pdu_clone != NULL) {
119+- /*
120+- * call security model if it needs to know about this
121+- */
122+- (*sptr->pdu_clone) (pdu, newpdu);
123+- }
124+-
125+ return newpdu;
126+ }
127+
128+diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
129+index ec2074c..ba4ca5f 100644
130+--- a/snmplib/snmpusm.c
131++++ b/snmplib/snmpusm.c
132+@@ -85,6 +85,7 @@ netsnmp_feature_child_of(usm_support, usm_all)
133+ netsnmp_feature_require(usm_support)
134+
135+ struct usmStateReference {
136++ int refcnt;
137+ char *usr_name;
138+ size_t usr_name_length;
139+ u_char *usr_engine_id;
140+@@ -301,43 +302,63 @@ free_enginetime_on_shutdown(int majorid, int minorid, void *serverarg,
141+ struct usmStateReference *
142+ usm_malloc_usmStateReference(void)
143+ {
144+- struct usmStateReference *retval = (struct usmStateReference *)
145+- calloc(1, sizeof(struct usmStateReference));
146++ struct usmStateReference *retval;
147++
148++ retval = calloc(1, sizeof(struct usmStateReference));
149++ if (retval)
150++ retval->refcnt = 1;
151+
152+ return retval;
153+ } /* end usm_malloc_usmStateReference() */
154+
155++static int
156++usm_clone(netsnmp_pdu *pdu, netsnmp_pdu *new_pdu)
157++{
158++ struct usmStateReference *ref = pdu->securityStateRef;
159++ struct usmStateReference **new_ref =
160++ (struct usmStateReference **)&new_pdu->securityStateRef;
161++ int ret = 0;
162++
163++ if (!ref)
164++ return ret;
165++
166++ if (pdu->command == SNMP_MSG_TRAP2) {
167++ netsnmp_assert(pdu->securityModel == SNMP_DEFAULT_SECMODEL);
168++ ret = usm_clone_usmStateReference(ref, new_ref);
169++ } else {
170++ netsnmp_assert(ref == *new_ref);
171++ ref->refcnt++;
172++ }
173++
174++ return ret;
175++}
176+
177+ void
178+ usm_free_usmStateReference(void *old)
179+ {
180+- struct usmStateReference *old_ref = (struct usmStateReference *) old;
181+-
182+- if (old_ref) {
183+-
184+- if (old_ref->usr_name_length)
185+- SNMP_FREE(old_ref->usr_name);
186+- if (old_ref->usr_engine_id_length)
187+- SNMP_FREE(old_ref->usr_engine_id);
188+- if (old_ref->usr_auth_protocol_length)
189+- SNMP_FREE(old_ref->usr_auth_protocol);
190+- if (old_ref->usr_priv_protocol_length)
191+- SNMP_FREE(old_ref->usr_priv_protocol);
192+-
193+- if (old_ref->usr_auth_key_length && old_ref->usr_auth_key) {
194+- SNMP_ZERO(old_ref->usr_auth_key, old_ref->usr_auth_key_length);
195+- SNMP_FREE(old_ref->usr_auth_key);
196+- }
197+- if (old_ref->usr_priv_key_length && old_ref->usr_priv_key) {
198+- SNMP_ZERO(old_ref->usr_priv_key, old_ref->usr_priv_key_length);
199+- SNMP_FREE(old_ref->usr_priv_key);
200+- }
201++ struct usmStateReference *ref = old;
202++
203++ if (!ref)
204++ return;
205+
206+- SNMP_ZERO(old_ref, sizeof(*old_ref));
207+- SNMP_FREE(old_ref);
208++ if (--ref->refcnt > 0)
209++ return;
210++
211++ SNMP_FREE(ref->usr_name);
212++ SNMP_FREE(ref->usr_engine_id);
213++ SNMP_FREE(ref->usr_auth_protocol);
214++ SNMP_FREE(ref->usr_priv_protocol);
215+
216++ if (ref->usr_auth_key_length && ref->usr_auth_key) {
217++ SNMP_ZERO(ref->usr_auth_key, ref->usr_auth_key_length);
218++ SNMP_FREE(ref->usr_auth_key);
219++ }
220++ if (ref->usr_priv_key_length && ref->usr_priv_key) {
221++ SNMP_ZERO(ref->usr_priv_key, ref->usr_priv_key_length);
222++ SNMP_FREE(ref->usr_priv_key);
223+ }
224+
225++ SNMP_FREE(ref);
226+ } /* end usm_free_usmStateReference() */
227+
228+ struct usmUser *
229+@@ -3337,6 +3358,7 @@ init_usm(void)
230+ def->encode_reverse = usm_secmod_rgenerate_out_msg;
231+ def->encode_forward = usm_secmod_generate_out_msg;
232+ def->decode = usm_secmod_process_in_msg;
233++ def->pdu_clone = usm_clone;
234+ def->pdu_free_state_ref = usm_free_usmStateReference;
235+ def->session_setup = usm_session_init;
236+ def->handle_report = usm_handle_report;
237+@@ -5110,4 +5132,3 @@ get_default_privtype(size_t * len)
238+ *len = defaultPrivTypeLen;
239+ return defaultPrivType;
240+ }
241+-
242diff --git a/debian/patches/move-free-securityStateRef-into-snmp-free-pdu.patch b/debian/patches/move-free-securityStateRef-into-snmp-free-pdu.patch
243new file mode 100644
244index 0000000..a568095
245--- /dev/null
246+++ b/debian/patches/move-free-securityStateRef-into-snmp-free-pdu.patch
247@@ -0,0 +1,92 @@
248+From: Bart Van Assche <bvanassche@acm.org>
249+Date: Tue, 13 Aug 2019 20:54:23 -0700
250+Subject: libsnmp: Move the free_securityStateRef() call into snmp_free_pdu()
251+
252+This patch fixes a memory leak that was introduced in commit 5f881d3bf245
253+("libsnmp, USM: Introduce a reference count in struct usmStateReference").
254+
255+This patch partially reverts commit adc9b71aba91 ("snmpd: Avoid that snmpv3
256+bulkget errors result in a double free").
257+
258+See also https://sourceforge.net/p/net-snmp/bugs/2938/.
259+
260+Author: Bart Van Assche <bvanassche@acm.org>
261+Origin: backport, https://github.com/net-snmp/net-snmp/commit/87bd90d04
262+Last-Update: 2020-06-17
263+Bug: https://sourceforge.net/p/net-snmp/bugs/2938/
264+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
265+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
266+---
267+ include/net-snmp/pdu_api.h | 2 --
268+ snmplib/snmp_api.c | 24 +++---------------------
269+ 2 files changed, 3 insertions(+), 23 deletions(-)
270+
271+diff --git a/include/net-snmp/pdu_api.h b/include/net-snmp/pdu_api.h
272+index 270aff0..125595d 100644
273+--- a/include/net-snmp/pdu_api.h
274++++ b/include/net-snmp/pdu_api.h
275+@@ -19,8 +19,6 @@ NETSNMP_IMPORT
276+ netsnmp_pdu *snmp_fix_pdu( netsnmp_pdu *pdu, int idx);
277+ NETSNMP_IMPORT
278+ void snmp_free_pdu( netsnmp_pdu *pdu);
279+-NETSNMP_IMPORT
280+-void snmp_free_securityStateRef( netsnmp_pdu *pdu);
281+
282+ #ifdef __cplusplus
283+ }
284+diff --git a/snmplib/snmp_api.c b/snmplib/snmp_api.c
285+index 046b247..0fea19e 100644
286+--- a/snmplib/snmp_api.c
287++++ b/snmplib/snmp_api.c
288+@@ -4033,17 +4033,6 @@ free_securityStateRef(netsnmp_pdu* pdu)
289+ pdu->securityStateRef = NULL;
290+ }
291+
292+-/*
293+- * This function is here to provide a separate call to
294+- * free the securityStateRef memory. This is needed to prevent
295+- * a double free if this memory is freed in snmp_free_pdu.
296+- */
297+-void
298+-snmp_free_securityStateRef(netsnmp_pdu* pdu)
299+-{
300+- free_securityStateRef(pdu);
301+-}
302+-
303+ #define ERROR_STAT_LENGTH 11
304+
305+ int
306+@@ -5493,6 +5482,9 @@ snmp_free_pdu(netsnmp_pdu *pdu)
307+ return;
308+ }
309+ */
310++
311++ free_securityStateRef(pdu);
312++
313+ if ((sptr = find_sec_mod(pdu->securityModel)) != NULL &&
314+ sptr->pdu_free != NULL) {
315+ (*sptr->pdu_free) (pdu);
316+@@ -5647,10 +5639,6 @@ _sess_process_packet_parse_pdu(void *sessp, netsnmp_session * sp,
317+ }
318+
319+ if (ret != SNMP_ERR_NOERROR) {
320+- /*
321+- * Call the security model to free any securityStateRef supplied w/ msg.
322+- */
323+- free_securityStateRef(pdu);
324+ snmp_free_pdu(pdu);
325+ return NULL;
326+ }
327+@@ -5822,12 +5810,6 @@ _sess_process_packet_handle_pdu(void *sessp, netsnmp_session * sp,
328+ }
329+ }
330+
331+- /*
332+- * Call USM to free any securityStateRef supplied with the message.
333+- */
334+- if (pdu->command == SNMP_MSG_TRAP2)
335+- free_securityStateRef(pdu);
336+-
337+ if (!handled) {
338+ if (sp->flags & SNMP_FLAGS_SHARED_SOCKET)
339+ return -2;
340diff --git a/debian/patches/move-securityStateRef-into-free_securityStateRef.patch b/debian/patches/move-securityStateRef-into-free_securityStateRef.patch
341new file mode 100644
342index 0000000..fe6697c
343--- /dev/null
344+++ b/debian/patches/move-securityStateRef-into-free_securityStateRef.patch
345@@ -0,0 +1,78 @@
346+From: Bart Van Assche <bvanassche@acm.org>
347+Date: Sat, 25 May 2019 16:33:31 +0200
348+Subject: libsnmp: Move the securityStateRef check into
349+ free_securityStateRef()
350+
351+Instead of making each free_securityStateRef() caller check the
352+securityStateRef pointer, move that check into free_securityStateRef().
353+
354+Author: Bart Van Assche <bvanassche@acm.org>
355+Origin: backport, https://github.com/net-snmp/net-snmp/commit/92ccd5a82
356+Last-Update: 2020-06-17
357+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
358+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
359+---
360+ agent/snmp_agent.c | 1 +
361+ snmplib/snmp_api.c | 21 ++++++++++-----------
362+ 2 files changed, 11 insertions(+), 11 deletions(-)
363+
364+diff --git a/snmplib/snmp_api.c b/snmplib/snmp_api.c
365+index 6754fa6..bfc052b 100644
366+--- a/snmplib/snmp_api.c
367++++ b/snmplib/snmp_api.c
368+@@ -4011,7 +4011,12 @@ snmpv3_parse(netsnmp_pdu *pdu,
369+ static void
370+ free_securityStateRef(netsnmp_pdu* pdu)
371+ {
372+- struct snmp_secmod_def *sptr = find_sec_mod(pdu->securityModel);
373++ struct snmp_secmod_def *sptr;
374++
375++ if (!pdu->securityStateRef)
376++ return;
377++
378++ sptr = find_sec_mod(pdu->securityModel);
379+ if (sptr) {
380+ if (sptr->pdu_free_state_ref) {
381+ (*sptr->pdu_free_state_ref) (pdu->securityStateRef);
382+@@ -4122,9 +4127,7 @@ snmpv3_make_report(netsnmp_pdu *pdu, int error)
383+ * FIX - yes they should but USM needs to follow new EoP to determine
384+ * which cached values to use
385+ */
386+- if (pdu->securityStateRef) {
387+- free_securityStateRef(pdu);
388+- }
389++ free_securityStateRef(pdu);
390+
391+ if (error == SNMPERR_USM_NOTINTIMEWINDOW) {
392+ pdu->securityLevel = SNMP_SEC_LEVEL_AUTHNOPRIV;
393+@@ -5636,9 +5639,7 @@ _sess_process_packet_parse_pdu(void *sessp, netsnmp_session * sp,
394+ /*
395+ * Call the security model to free any securityStateRef supplied w/ msg.
396+ */
397+- if (pdu->securityStateRef != NULL) {
398+- free_securityStateRef(pdu);
399+- }
400++ free_securityStateRef(pdu);
401+ snmp_free_pdu(pdu);
402+ return NULL;
403+ }
404+@@ -5662,9 +5663,7 @@ _sess_process_packet_handle_pdu(void *sessp, netsnmp_session * sp,
405+ /*
406+ * Call USM to free any securityStateRef supplied with the message.
407+ */
408+- if (pdu->securityStateRef) {
409+- free_securityStateRef(pdu);
410+- }
411++ free_securityStateRef(pdu);
412+
413+ for (rp = isp->requests; rp; orp = rp, rp = rp->next_request) {
414+ snmp_callback callback;
415+@@ -5815,7 +5814,7 @@ _sess_process_packet_handle_pdu(void *sessp, netsnmp_session * sp,
416+ /*
417+ * Call USM to free any securityStateRef supplied with the message.
418+ */
419+- if (pdu->securityStateRef && pdu->command == SNMP_MSG_TRAP2)
420++ if (pdu->command == SNMP_MSG_TRAP2)
421+ free_securityStateRef(pdu);
422+
423+ if (!handled) {
424diff --git a/debian/patches/prevent-snmpv3-bulkget-errors-double-free.patch b/debian/patches/prevent-snmpv3-bulkget-errors-double-free.patch
425new file mode 100644
426index 0000000..61b8348
427--- /dev/null
428+++ b/debian/patches/prevent-snmpv3-bulkget-errors-double-free.patch
429@@ -0,0 +1,349 @@
430+From: Sam Tannous <stannous@cumulusnetworks.com>
431+Date: Wed, 10 Apr 2019 06:57:21 -0700
432+Subject: snmpd: Avoid that snmpv3 bulkget errors result in a double free
433+
434+See also https://sourceforge.net/p/net-snmp/bugs/2923/.
435+See also https://sourceforge.net/p/net-snmp/patches/1388/.
436+
437+Author: Ming Chen <ming0903@users.sourceforge.net>
438+Origin: upstream, https://github.com/net-snmp/net-snmp/commit/adc9b71ab
439+Last-Update: 2020-06-17
440+Bug: https://sourceforge.net/p/net-snmp/bugs/2923/
441+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
442+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
443+---
444+ include/net-snmp/pdu_api.h | 2 ++
445+ snmplib/snmp_api.c | 11 ++++++++++
446+ snmplib/snmpusm.c | 51 ++++++++++------------------------------------
447+ 3 files changed, 24 insertions(+), 40 deletions(-)
448+
449+diff --git a/include/net-snmp/pdu_api.h b/include/net-snmp/pdu_api.h
450+index 125595d..270aff0 100644
451+--- a/include/net-snmp/pdu_api.h
452++++ b/include/net-snmp/pdu_api.h
453+@@ -19,6 +19,8 @@ NETSNMP_IMPORT
454+ netsnmp_pdu *snmp_fix_pdu( netsnmp_pdu *pdu, int idx);
455+ NETSNMP_IMPORT
456+ void snmp_free_pdu( netsnmp_pdu *pdu);
457++NETSNMP_IMPORT
458++void snmp_free_securityStateRef( netsnmp_pdu *pdu);
459+
460+ #ifdef __cplusplus
461+ }
462+diff --git a/snmplib/snmp_api.c b/snmplib/snmp_api.c
463+index bfc052b..046b247 100644
464+--- a/snmplib/snmp_api.c
465++++ b/snmplib/snmp_api.c
466+@@ -4033,6 +4033,17 @@ free_securityStateRef(netsnmp_pdu* pdu)
467+ pdu->securityStateRef = NULL;
468+ }
469+
470++/*
471++ * This function is here to provide a separate call to
472++ * free the securityStateRef memory. This is needed to prevent
473++ * a double free if this memory is freed in snmp_free_pdu.
474++ */
475++void
476++snmp_free_securityStateRef(netsnmp_pdu* pdu)
477++{
478++ free_securityStateRef(pdu);
479++}
480++
481+ #define ERROR_STAT_LENGTH 11
482+
483+ int
484+diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
485+index e697c23..c1a6dcc 100644
486+--- a/snmplib/snmpusm.c
487++++ b/snmplib/snmpusm.c
488+@@ -299,16 +299,20 @@ usm_free_usmStateReference(void *old)
489+
490+ if (old_ref) {
491+
492+- SNMP_FREE(old_ref->usr_name);
493+- SNMP_FREE(old_ref->usr_engine_id);
494+- SNMP_FREE(old_ref->usr_auth_protocol);
495+- SNMP_FREE(old_ref->usr_priv_protocol);
496+-
497+- if (old_ref->usr_auth_key) {
498++ if (old_ref->usr_name_length)
499++ SNMP_FREE(old_ref->usr_name);
500++ if (old_ref->usr_engine_id_length)
501++ SNMP_FREE(old_ref->usr_engine_id);
502++ if (old_ref->usr_auth_protocol_length)
503++ SNMP_FREE(old_ref->usr_auth_protocol);
504++ if (old_ref->usr_auth_protocol_length)
505++ SNMP_FREE(old_ref->usr_priv_protocol);
506++
507++ if (old_ref->usr_auth_key_length && old_ref->usr_auth_key) {
508+ SNMP_ZERO(old_ref->usr_auth_key, old_ref->usr_auth_key_length);
509+ SNMP_FREE(old_ref->usr_auth_key);
510+ }
511+- if (old_ref->usr_priv_key) {
512++ if (old_ref->usr_priv_key_length && old_ref->usr_priv_key) {
513+ SNMP_ZERO(old_ref->usr_priv_key, old_ref->usr_priv_key_length);
514+ SNMP_FREE(old_ref->usr_priv_key);
515+ }
516+@@ -1039,7 +1043,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
517+ if ((user = usm_get_user(secEngineID, secEngineIDLen, secName))
518+ == NULL && secLevel != SNMP_SEC_LEVEL_NOAUTH) {
519+ DEBUGMSGTL(("usm", "Unknown User(%s)\n", secName));
520+- usm_free_usmStateReference(secStateRef);
521+ return SNMPERR_USM_UNKNOWNSECURITYNAME;
522+ }
523+
524+@@ -1091,7 +1094,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
525+ thePrivProtocolLength) == 1) {
526+ DEBUGMSGTL(("usm", "Unsupported Security Level (%d)\n",
527+ theSecLevel));
528+- usm_free_usmStateReference(secStateRef);
529+ return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL;
530+ }
531+
532+@@ -1121,7 +1123,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
533+ &msgAuthParmLen, &msgPrivParmLen, &otstlen,
534+ &seq_len, &msgSecParmLen) == -1) {
535+ DEBUGMSGTL(("usm", "Failed calculating offsets.\n"));
536+- usm_free_usmStateReference(secStateRef);
537+ return SNMPERR_USM_GENERICERROR;
538+ }
539+
540+@@ -1143,7 +1144,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
541+ ptr = *wholeMsg = globalData;
542+ if (theTotalLength > *wholeMsgLen) {
543+ DEBUGMSGTL(("usm", "Message won't fit in buffer.\n"));
544+- usm_free_usmStateReference(secStateRef);
545+ return SNMPERR_USM_GENERICERROR;
546+ }
547+
548+@@ -1169,7 +1169,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
549+ htonl(boots_uint), htonl(time_uint),
550+ &ptr[privParamsOffset]) == -1) {
551+ DEBUGMSGTL(("usm", "Can't set AES iv.\n"));
552+- usm_free_usmStateReference(secStateRef);
553+ return SNMPERR_USM_GENERICERROR;
554+ }
555+ }
556+@@ -1185,7 +1184,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
557+ &ptr[privParamsOffset])
558+ == -1)) {
559+ DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n"));
560+- usm_free_usmStateReference(secStateRef);
561+ return SNMPERR_USM_GENERICERROR;
562+ }
563+ }
564+@@ -1198,7 +1196,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
565+ &ptr[dataOffset], &encrypted_length)
566+ != SNMP_ERR_NOERROR) {
567+ DEBUGMSGTL(("usm", "encryption error.\n"));
568+- usm_free_usmStateReference(secStateRef);
569+ return SNMPERR_USM_ENCRYPTIONERROR;
570+ }
571+ #ifdef NETSNMP_ENABLE_TESTING_CODE
572+@@ -1226,7 +1223,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
573+ if ((encrypted_length != (theTotalLength - dataOffset))
574+ || (salt_length != msgPrivParmLen)) {
575+ DEBUGMSGTL(("usm", "encryption length error.\n"));
576+- usm_free_usmStateReference(secStateRef);
577+ return SNMPERR_USM_ENCRYPTIONERROR;
578+ }
579+
580+@@ -1362,7 +1358,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
581+
582+ if (temp_sig == NULL) {
583+ DEBUGMSGTL(("usm", "Out of memory.\n"));
584+- usm_free_usmStateReference(secStateRef);
585+ return SNMPERR_USM_GENERICERROR;
586+ }
587+
588+@@ -1376,7 +1371,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
589+ SNMP_ZERO(temp_sig, temp_sig_len);
590+ SNMP_FREE(temp_sig);
591+ DEBUGMSGTL(("usm", "Signing failed.\n"));
592+- usm_free_usmStateReference(secStateRef);
593+ return SNMPERR_USM_AUTHENTICATIONFAILURE;
594+ }
595+
596+@@ -1384,7 +1378,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
597+ SNMP_ZERO(temp_sig, temp_sig_len);
598+ SNMP_FREE(temp_sig);
599+ DEBUGMSGTL(("usm", "Signing lengths failed.\n"));
600+- usm_free_usmStateReference(secStateRef);
601+ return SNMPERR_USM_AUTHENTICATIONFAILURE;
602+ }
603+
604+@@ -1398,7 +1391,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
605+ /*
606+ * endif -- create keyed hash
607+ */
608+- usm_free_usmStateReference(secStateRef);
609+
610+ DEBUGMSGTL(("usm", "USM processing completed.\n"));
611+
612+@@ -1548,7 +1540,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
613+ if ((user = usm_get_user(secEngineID, secEngineIDLen, secName))
614+ == NULL && secLevel != SNMP_SEC_LEVEL_NOAUTH) {
615+ DEBUGMSGTL(("usm", "Unknown User\n"));
616+- usm_free_usmStateReference(secStateRef);
617+ return SNMPERR_USM_UNKNOWNSECURITYNAME;
618+ }
619+
620+@@ -1601,7 +1592,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
621+ DEBUGMSGTL(("usm", "Unsupported Security Level or type (%d)\n",
622+ theSecLevel));
623+
624+- usm_free_usmStateReference(secStateRef);
625+ return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL;
626+ }
627+
628+@@ -1636,7 +1626,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
629+ DEBUGMSGTL(("usm",
630+ "couldn't malloc %d bytes for encrypted PDU\n",
631+ (int)ciphertextlen));
632+- usm_free_usmStateReference(secStateRef);
633+ return SNMPERR_MALLOC;
634+ }
635+
636+@@ -1652,7 +1641,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
637+ htonl(boots_uint), htonl(time_uint),
638+ iv) == -1) {
639+ DEBUGMSGTL(("usm", "Can't set AES iv.\n"));
640+- usm_free_usmStateReference(secStateRef);
641+ SNMP_FREE(ciphertext);
642+ return SNMPERR_USM_GENERICERROR;
643+ }
644+@@ -1667,7 +1655,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
645+ thePrivKeyLength - 8,
646+ iv) == -1)) {
647+ DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n"));
648+- usm_free_usmStateReference(secStateRef);
649+ SNMP_FREE(ciphertext);
650+ return SNMPERR_USM_GENERICERROR;
651+ }
652+@@ -1686,7 +1673,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
653+ scopedPdu, scopedPduLen,
654+ ciphertext, &ciphertextlen) != SNMP_ERR_NOERROR) {
655+ DEBUGMSGTL(("usm", "encryption error.\n"));
656+- usm_free_usmStateReference(secStateRef);
657+ SNMP_FREE(ciphertext);
658+ return SNMPERR_USM_ENCRYPTIONERROR;
659+ }
660+@@ -1703,7 +1689,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
661+ ciphertext, ciphertextlen);
662+ if (rc == 0) {
663+ DEBUGMSGTL(("usm", "Encryption failed.\n"));
664+- usm_free_usmStateReference(secStateRef);
665+ SNMP_FREE(ciphertext);
666+ return SNMPERR_USM_ENCRYPTIONERROR;
667+ }
668+@@ -1743,7 +1728,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
669+ DEBUGINDENTLESS();
670+ if (rc == 0) {
671+ DEBUGMSGTL(("usm", "building privParams failed.\n"));
672+- usm_free_usmStateReference(secStateRef);
673+ return SNMPERR_TOO_LONG;
674+ }
675+
676+@@ -1766,7 +1750,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
677+ DEBUGINDENTLESS();
678+ if (rc == 0) {
679+ DEBUGMSGTL(("usm", "building authParams failed.\n"));
680+- usm_free_usmStateReference(secStateRef);
681+ return SNMPERR_TOO_LONG;
682+ }
683+
684+@@ -1789,7 +1772,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
685+ DEBUGINDENTLESS();
686+ if (rc == 0) {
687+ DEBUGMSGTL(("usm", "building authParams failed.\n"));
688+- usm_free_usmStateReference(secStateRef);
689+ return SNMPERR_TOO_LONG;
690+ }
691+
692+@@ -1805,7 +1787,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
693+ if (rc == 0) {
694+ DEBUGMSGTL(("usm",
695+ "building msgAuthoritativeEngineTime failed.\n"));
696+- usm_free_usmStateReference(secStateRef);
697+ return SNMPERR_TOO_LONG;
698+ }
699+
700+@@ -1821,7 +1802,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
701+ if (rc == 0) {
702+ DEBUGMSGTL(("usm",
703+ "building msgAuthoritativeEngineBoots failed.\n"));
704+- usm_free_usmStateReference(secStateRef);
705+ return SNMPERR_TOO_LONG;
706+ }
707+
708+@@ -1833,7 +1813,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
709+ DEBUGINDENTLESS();
710+ if (rc == 0) {
711+ DEBUGMSGTL(("usm", "building msgAuthoritativeEngineID failed.\n"));
712+- usm_free_usmStateReference(secStateRef);
713+ return SNMPERR_TOO_LONG;
714+ }
715+
716+@@ -1846,7 +1825,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
717+ *offset - sp_offset);
718+ if (rc == 0) {
719+ DEBUGMSGTL(("usm", "building usm security parameters failed.\n"));
720+- usm_free_usmStateReference(secStateRef);
721+ return SNMPERR_TOO_LONG;
722+ }
723+
724+@@ -1860,7 +1838,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
725+
726+ if (rc == 0) {
727+ DEBUGMSGTL(("usm", "building msgSecurityParameters failed.\n"));
728+- usm_free_usmStateReference(secStateRef);
729+ return SNMPERR_TOO_LONG;
730+ }
731+
732+@@ -1870,7 +1847,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
733+ while ((*wholeMsgLen - *offset) < globalDataLen) {
734+ if (!asn_realloc(wholeMsg, wholeMsgLen)) {
735+ DEBUGMSGTL(("usm", "building global data failed.\n"));
736+- usm_free_usmStateReference(secStateRef);
737+ return SNMPERR_TOO_LONG;
738+ }
739+ }
740+@@ -1886,7 +1862,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
741+ ASN_CONSTRUCTOR), *offset);
742+ if (rc == 0) {
743+ DEBUGMSGTL(("usm", "building master packet sequence failed.\n"));
744+- usm_free_usmStateReference(secStateRef);
745+ return SNMPERR_TOO_LONG;
746+ }
747+
748+@@ -1904,7 +1879,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
749+
750+ if (temp_sig == NULL) {
751+ DEBUGMSGTL(("usm", "Out of memory.\n"));
752+- usm_free_usmStateReference(secStateRef);
753+ return SNMPERR_USM_GENERICERROR;
754+ }
755+
756+@@ -1915,14 +1889,12 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
757+ != SNMP_ERR_NOERROR) {
758+ SNMP_FREE(temp_sig);
759+ DEBUGMSGTL(("usm", "Signing failed.\n"));
760+- usm_free_usmStateReference(secStateRef);
761+ return SNMPERR_USM_AUTHENTICATIONFAILURE;
762+ }
763+
764+ if (temp_sig_len != msgAuthParmLen) {
765+ SNMP_FREE(temp_sig);
766+ DEBUGMSGTL(("usm", "Signing lengths failed.\n"));
767+- usm_free_usmStateReference(secStateRef);
768+ return SNMPERR_USM_AUTHENTICATIONFAILURE;
769+ }
770+
771+@@ -1933,7 +1905,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
772+ /*
773+ * endif -- create keyed hash
774+ */
775+- usm_free_usmStateReference(secStateRef);
776+ DEBUGMSGTL(("usm", "USM processing completed.\n"));
777+ return SNMPERR_SUCCESS;
778+ } /* end usm_rgenerate_out_msg() */
779diff --git a/debian/patches/series b/debian/patches/series
780index 8160545..eac6e7a 100644
781--- a/debian/patches/series
782+++ b/debian/patches/series
783@@ -37,3 +37,9 @@ autofs-skip-autofs-entries.patch
784 autofs-fix-a-recently-introduced-bug.patch
785 fix-check-hr-filesys-autofs.patch
786 lp1871307-log-once-proc-net-if_inet6-failure.patch
787+move-securityStateRef-into-free_securityStateRef.patch
788+prevent-snmpv3-bulkget-errors-double-free.patch
789+fix-usmStateReference-free.patch
790+unexport-struct-usmStateReference.patch
791+introduce-refcount-usmStateReference.patch
792+move-free-securityStateRef-into-snmp-free-pdu.patch
793diff --git a/debian/patches/unexport-struct-usmStateReference.patch b/debian/patches/unexport-struct-usmStateReference.patch
794new file mode 100644
795index 0000000..aeb3081
796--- /dev/null
797+++ b/debian/patches/unexport-struct-usmStateReference.patch
798@@ -0,0 +1,74 @@
799+From: Bart Van Assche <bvanassche@acm.org>
800+Date: Tue, 23 Jul 2019 10:52:28 -0700
801+Subject: libsnmp: Unexport struct usmStateReference
802+
803+Certain snmpd crashes can only be fixed by introducing a reference
804+count in struct usmStateReference. Unexport that structure such that
805+changing it does not affect the ABI.
806+
807+Author: Bart Van Assche <bvanassche@acm.org>
808+Origin: upstream, https://github.com/net-snmp/net-snmp/commit/39381c4d2
809+Last-Update: 2020-06-17
810+Bug: https://sourceforge.net/p/net-snmp/bugs/2956/
811+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/net-snmp/+bug/1877027
812+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
813+---
814+ include/net-snmp/library/snmpusm.h | 17 +----------------
815+ snmplib/snmpusm.c | 16 ++++++++++++++++
816+ 2 files changed, 17 insertions(+), 16 deletions(-)
817+
818+diff --git a/include/net-snmp/library/snmpusm.h b/include/net-snmp/library/snmpusm.h
819+index 3f60787..49061d8 100644
820+--- a/include/net-snmp/library/snmpusm.h
821++++ b/include/net-snmp/library/snmpusm.h
822+@@ -42,22 +42,7 @@ extern "C" {
823+ /*
824+ * Structures.
825+ */
826+- struct usmStateReference {
827+- char *usr_name;
828+- size_t usr_name_length;
829+- u_char *usr_engine_id;
830+- size_t usr_engine_id_length;
831+- oid *usr_auth_protocol;
832+- size_t usr_auth_protocol_length;
833+- u_char *usr_auth_key;
834+- size_t usr_auth_key_length;
835+- oid *usr_priv_protocol;
836+- size_t usr_priv_protocol_length;
837+- u_char *usr_priv_key;
838+- size_t usr_priv_key_length;
839+- u_int usr_sec_level;
840+- };
841+-
842++ struct usmStateReference;
843+
844+ /*
845+ * struct usmUser: a structure to represent a given user in a list
846+diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
847+index 0853e8d..ec2074c 100644
848+--- a/snmplib/snmpusm.c
849++++ b/snmplib/snmpusm.c
850+@@ -84,6 +84,22 @@ netsnmp_feature_child_of(usm_support, usm_all)
851+
852+ netsnmp_feature_require(usm_support)
853+
854++struct usmStateReference {
855++ char *usr_name;
856++ size_t usr_name_length;
857++ u_char *usr_engine_id;
858++ size_t usr_engine_id_length;
859++ oid *usr_auth_protocol;
860++ size_t usr_auth_protocol_length;
861++ u_char *usr_auth_key;
862++ size_t usr_auth_key_length;
863++ oid *usr_priv_protocol;
864++ size_t usr_priv_protocol_length;
865++ u_char *usr_priv_key;
866++ size_t usr_priv_key_length;
867++ u_int usr_sec_level;
868++};
869++
870+ oid usmNoAuthProtocol[10] = { NETSNMP_USMAUTH_BASE_OID,
871+ NETSNMP_USMAUTH_NOAUTH };
872+ #ifndef NETSNMP_DISABLE_MD5

Subscribers

People subscribed via source and target branches