search

markabrahams / node-net-snmp

JavaScript implementation of the Simple Network Management Protocol (SNMP)
206 stars 97 forks source link

Implement SHA2, fix DES, and some crypto-related cleanups #204

Closed mvduin closed 2 years ago

mvduin commented 2 years ago

This is a set of improvements to the SNMPv3 cryptography code, including

markabrahams commented 2 years ago

Hi @mvduin - thanks heaps for the PR! Good code improvements, and nice to add SHA-2 protocol support!

What I've done is merge all of the commits except for the DES auto-padding one, which I've reverted. This commit re-introduced some decoding problems that the original workaround addressed. From memory, I think on some occasions extra padding was introduced that wasn't handled by the packet decoder. The workaround wasn't a great one, but it did resolve the problem. I suspect a better solution than that existing workaround would be to detect the extra padding post-decrypt, and then strip that out before trying to decode.

Although the problem is occasional and data-dependent, I've always managed to reproduce it by doing a session.walk() of the entire MIB of a fairly standard Linux Net-SNMP agent (running on localhost).

e.g. with this PR, this sample program will always fail with DES:

node ./example/snmp-walk.js -v 3 -u wilma -l authPriv -a sha -A <authpass> -x des -X <privpass> localhost 1.3.6.1

The problem is that certain GetBulk responses would be decrypted/decoded incorrectly to have a extra few varbinds with nulls added, presumably because of the failure to handle certain padding scenarios properly.

e.g. for my agent, with the current master, this sample program runs fine:

mark@hope:~/node-net-snmp$ node ./example/snmp-get-bulk.js -v 3 -u wilma -l authPriv -a sha -A <authpass> -x des -X <privpass> -n 0 -r 20 localhost 1.3.6.1.2.1.1.9.1.3.2
1.3.6.1.2.1.1.9.1.3.3|The management information definitions for the SNMP User-based Security Model.
1.3.6.1.2.1.1.9.1.3.4|The MIB module for SNMPv2 entities
1.3.6.1.2.1.1.9.1.3.5|View-based Access Control Model for SNMP.
1.3.6.1.2.1.1.9.1.3.6|The MIB module for managing TCP implementations
1.3.6.1.2.1.1.9.1.3.7|The MIB module for managing IP and ICMP implementations
1.3.6.1.2.1.1.9.1.3.8|The MIB module for managing UDP implementations
1.3.6.1.2.1.1.9.1.3.9|The MIB modules for managing SNMP Notification, plus filtering.
1.3.6.1.2.1.1.9.1.3.10|The MIB module for logging SNMP Notifications.
1.3.6.1.2.1.1.9.1.3.11|The MIB module for logging SNMP Notifications.
1.3.6.1.2.1.1.9.1.4.1|0
1.3.6.1.2.1.1.9.1.4.2|0
1.3.6.1.2.1.1.9.1.4.3|0
1.3.6.1.2.1.1.9.1.4.4|0
1.3.6.1.2.1.1.9.1.4.5|0
1.3.6.1.2.1.1.9.1.4.6|0
1.3.6.1.2.1.1.9.1.4.7|0
1.3.6.1.2.1.1.9.1.4.8|0
1.3.6.1.2.1.1.9.1.4.9|0
1.3.6.1.2.1.1.9.1.4.10|0
1.3.6.1.2.1.1.9.1.4.11|1498

But with this PR, running the same thing, a couple of spurious varbinds with nulls get added:

mark@hope:~/node-net-snmp$ node ./example/snmp-get-bulk.js -v 3 -u wilma -l authPriv -a sha -A <authpass> -x des -X <privpass> -n 0 -r 20 localhost 1.3.6.1.2.1.1.9.1.3.2
1.3.6.1.2.1.1.9.1.3.3|The management information definitions for the SNMP User-based Security Model.
1.3.6.1.2.1.1.9.1.3.4|The MIB module for SNMPv2 entities
1.3.6.1.2.1.1.9.1.3.5|View-based Access Control Model for SNMP.
1.3.6.1.2.1.1.9.1.3.6|The MIB module for managing TCP implementations
1.3.6.1.2.1.1.9.1.3.7|The MIB module for managing IP and ICMP implementations
1.3.6.1.2.1.1.9.1.3.8|The MIB module for managing UDP implementations
1.3.6.1.2.1.1.9.1.3.9|The MIB modules for managing SNMP Notification, plus filtering.
1.3.6.1.2.1.1.9.1.3.10|The MIB module for logging SNMP Notifications.
1.3.6.1.2.1.1.9.1.3.11|The MIB module for logging SNMP Notifications.
1.3.6.1.2.1.1.9.1.4.1|0
1.3.6.1.2.1.1.9.1.4.2|0
1.3.6.1.2.1.1.9.1.4.3|0
1.3.6.1.2.1.1.9.1.4.4|0
1.3.6.1.2.1.1.9.1.4.5|0
1.3.6.1.2.1.1.9.1.4.6|0
1.3.6.1.2.1.1.9.1.4.7|0
1.3.6.1.2.1.1.9.1.4.8|0
1.3.6.1.2.1.1.9.1.4.9|0
1.3.6.1.2.1.1.9.1.4.10|0
1.3.6.1.2.1.1.9.1.4.11|1498
null|null
null|null

Being data-dependent, that exact part of the MIB may not reproduce the problem for you, but a full session.walk() should.

So I'm happy to merge a better fix for the workaround as long as we don't introduce this regression.

If you're keen to have a further look at this, it's probably cleanest to raise this as a new PR so it can be merged individually.

Thanks!

mvduin commented 2 years ago

Any padding added is outside the SEQUENCE of varbinds, outside the SEQUENCE of the pdu, outside the SEQUENCE of the contextPdu. If you're trying to decode the padding as if they're varbinds then that just means you're ignoring the lengths of these enclosing SEQUENCEs, which is a bug in the parser, not a problem of the decryption. With auto-padding enabled the decryption will sometimes strip off actual data (e.g. if the contextPdu happens to be a multiple of 8 bytes and the last byte is 01) which is not always caught by your ASN.1 parser either, I was getting a null in the place of the actual value.

mvduin commented 2 years ago

Note in particular that if the agent uses zero-padding then trying to decrypt with auto-padding enabled will never work correctly, it will either throw an exception or truncate the packet.

As a reminder of what RFC3414 says about padding for DES encryption: "Its length should be an integral multiple of 8 - and if it is not, the data is padded at the end as necessary. The actual pad value is irrelevant." and "When decrypting, the padding is ignored."

mvduin commented 2 years ago

Looking at the code, the ber.Reader API really seems to encourage bad behaviour by having readSequence just consume the tag and length and that's it. A solution is to just create a new reader for any sub-sequence instead, i.e. new ber.Reader( read.readString(tag | ber.Constructor, true) ) where tag is e.g. ber.Sequence or whatever the expected tag is.

Regardless of how exactly the parser is fixed, my DES patch should still be merged since without it the decryption is plain wrong and will truncate packets.

markabrahams commented 2 years ago

Yes - agreed that the parser has a bug that requires fixing. But I can't merge your decryption fix alone without the associated parser fix, as the decryption fix without the parser fix makes parse failures extremely common.

Do you have a way I can replicate your case of data being stripping and getting a null instead of the actual value? That would be helpful for testing. Thanks!

mvduin commented 2 years ago

It'll happen any time the ScopedPDU is a multiple of 8 bytes and final varbind is an OCTET STRING that ends in a 01 byte, unless the agent adds superfluous padding if the ScopedPDU is already a multiple of 8 bytes (which our switch does not).

Example of an actual ScopedPDU plaintext:

0000   30 82 02 cc 04 0b 80 00 11 ae 03 08 bd 43 6a a0
0010   30 04 00 a2 82 02 b9 02 04 01 47 57 8c 02 01 00
0020   02 01 00 30 82 02 a9 30 13 06 0e 28 c4 62 01 01
0030   02 01 04 01 01 04 4f 2d 01 02 01 07 30 32 06 0e
0040   28 c4 62 01 01 02 01 04 01 01 05 4f 2d 01 04 20
0050   39 64 33 30 37 64 65 39 35 32 66 39 39 66 30 33
0060   65 65 34 62 64 38 65 33 62 38 36 30 34 39 30 34
0070   30 13 06 0e 28 c4 62 01 01 02 01 04 01 01 06 4f
0080   2d 01 02 01 05 30 16 06 0e 28 c4 62 01 01 02 01
0090   04 01 01 07 4f 2d 01 04 04 65 74 68 30 30 12 06
00a0   0e 28 c4 62 01 01 02 01 04 01 01 08 4f 2d 01 04
00b0   00 30 18 06 0e 28 c4 62 01 01 02 01 04 01 01 09
00c0   4f 2d 01 04 06 38 63 2d 31 33 32 30 12 06 0e 28
00d0   c4 62 01 01 02 01 04 01 01 0a 4f 2d 01 04 00 30
00e0   14 06 0e 28 c4 62 01 01 02 01 04 01 01 0b 4f 2d
00f0   01 04 02 00 29 30 14 06 0e 28 c4 62 01 01 02 01
0100   04 01 01 0c 4f 2d 01 04 02 00 01 30 13 06 0e 28
0110   c4 62 01 01 02 01 04 01 01 04 4f 2f 02 02 01 07
0120   30 32 06 0e 28 c4 62 01 01 02 01 04 01 01 05 4f
0130   2f 02 04 20 32 38 39 66 36 34 66 34 39 62 64 64
0140   39 61 36 36 30 37 34 62 34 37 65 65 61 30 38 61
0150   35 39 61 62 30 13 06 0e 28 c4 62 01 01 02 01 04
0160   01 01 06 4f 2f 02 02 01 05 30 16 06 0e 28 c4 62
0170   01 01 02 01 04 01 01 07 4f 2f 02 04 04 65 74 68
0180   30 30 12 06 0e 28 c4 62 01 01 02 01 04 01 01 08
0190   4f 2f 02 04 00 30 18 06 0e 28 c4 62 01 01 02 01
01a0   04 01 01 09 4f 2f 02 04 06 38 63 2d 31 33 31 30
01b0   12 06 0e 28 c4 62 01 01 02 01 04 01 01 0a 4f 2f
01c0   02 04 00 30 14 06 0e 28 c4 62 01 01 02 01 04 01
01d0   01 0b 4f 2f 02 04 02 00 29 30 14 06 0e 28 c4 62
01e0   01 01 02 01 04 01 01 0c 4f 2f 02 04 02 00 01 30
01f0   13 06 0e 28 c4 62 01 01 02 01 04 01 01 04 50 26
0200   03 02 01 07 30 32 06 0e 28 c4 62 01 01 02 01 04
0210   01 01 05 50 26 03 04 20 30 38 38 35 64 33 39 38
0220   31 37 34 63 34 61 61 32 39 62 66 65 37 32 33 38
0230   33 37 63 62 62 62 61 36 30 13 06 0e 28 c4 62 01
0240   01 02 01 04 01 01 06 50 26 03 02 01 05 30 16 06
0250   0e 28 c4 62 01 01 02 01 04 01 01 07 50 26 03 04
0260   04 65 74 68 30 30 12 06 0e 28 c4 62 01 01 02 01
0270   04 01 01 08 50 26 03 04 00 30 15 06 0e 28 c4 62
0280   01 01 02 01 04 01 01 09 50 26 03 04 03 62 6f 62
0290   30 12 06 0e 28 c4 62 01 01 02 01 04 01 01 0a 50
02a0   26 03 04 00 30 14 06 0e 28 c4 62 01 01 02 01 04
02b0   01 01 0b 50 26 03 04 02 00 29 30 14 06 0e 28 c4
02c0   62 01 01 02 01 04 01 01 0c 50 26 03 04 02 00 01
mvduin commented 2 years ago

I think this should suffice to at least get the parser to strip off the padding?

--- i/index.js
+++ w/index.js
@@ -879,7 +879,7 @@ var readPdu = function (reader, scoped) {
    var contextEngineID;
    var contextName;
    if ( scoped ) {
-       reader.readSequence ();
+       reader = new ber.Reader (reader.readString (ber.Sequence | ber.Constructor, true));
        contextEngineID = reader.readString (ber.OctetString, true);
        contextName = reader.readString ();
    }
markabrahams commented 2 years ago

Great - thanks again for that @mvduin ! That checks out well in my testing.

I've merged into master and published as version 3.7.1 of the npm.