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