Bug #7902
openIKE invalid JSON log output
Description
I don't have a packet capture to reproduce this bug, but I have a credible bug report and a plausible explanation. The snippet below is a log output from an IKEv1 detection, but the JSON itself is not valid because it contains objects with duplicately-defined keys. The problematic keys in this example are sa_life_duration and sa_life_duration_raw which appear twice in some of the proposal objects. JSON parsers reading this as an input will either dedupe (quietly losing data), or fail with a parse error.
I can see how it seems possible for Suricata to produce an output like this - the add_attributes [1] iterates over a vector parsed from the IKE message and the JsonBuilder directly builds a JSON-formatted string from that input without validating, so when the transform vector contains duplicately-defined attributes it looks like both will come out in the output object.
Could you investigate to find a solution? I'm not deep enough in the details of IKEv1 to tell whether a repeated key in the proposal should be valid IKE or not, but I think regardless of invalid inputs Suricata should not produce an invalid JSON output. We have multiple reports of this happening so this is something that happens in the wild. The log below was produced using Suricata v7 but I see the same mechanism in the current master branch so I expect the same would happen in v8.
Log:
{
"event":{
"ike":{
"ikev1":{
"doi":1,
"encrypted_payloads":false,
"client":{
"proposals":[
{
"alg_enc":"EncAesCbc",
"alg_enc_raw":7,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":256,
"alg_hash":"HashSha2_384",
"alg_hash_raw":5,
"alg_auth":"AuthPreSharedKey",
"alg_auth_raw":1,
"alg_dh":"GroupRandomEcp384",
"alg_dh_raw":20,
"sa_life_type":"LifeTypeSeconds",
"sa_life_type_raw":1,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":65535
},
{
"alg_enc":"EncAesCbc",
"alg_enc_raw":7,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":256,
"alg_hash":"HashSha2_256",
"alg_hash_raw":4,
"alg_auth":"AuthPreSharedKey",
"alg_auth_raw":1,
"alg_dh":"GroupModp2048Bit",
"alg_dh_raw":14,
"sa_life_type":"LifeTypeSeconds",
"sa_life_type_raw":1,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":65535
},
{
"alg_enc":"EncAesCbc",
"alg_enc_raw":7,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":128,
"alg_hash":"HashSha",
"alg_hash_raw":2,
"alg_auth":"AuthPreSharedKey",
"alg_auth_raw":1,
"alg_dh":"GroupAlternate1024BitModpGroup",
"alg_dh_raw":2,
"sa_life_type":"LifeTypeSeconds",
"sa_life_type_raw":1,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":65535
},
{
"alg_enc":"EncTripleDesCbc",
"alg_enc_raw":5,
"alg_hash":"HashSha",
"alg_hash_raw":2,
"alg_auth":"AuthPreSharedKey",
"alg_auth_raw":1,
"alg_dh":"GroupAlternate1024BitModpGroup",
"alg_dh_raw":2,
"sa_life_type":"LifeTypeSeconds",
"sa_life_type_raw":1,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":65535
},
{
"alg_enc":"EncTripleDesCbc",
"alg_enc_raw":5,
"alg_hash":"HashMd5",
"alg_hash_raw":1,
"alg_auth":"AuthPreSharedKey",
"alg_auth_raw":1,
"alg_dh":"GroupAlternate1024BitModpGroup",
"alg_dh_raw":2,
"sa_life_type":"LifeTypeSeconds",
"sa_life_type_raw":1,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":65535
},
{
"alg_enc":"EncDesCbc",
"alg_enc_raw":1,
"alg_hash":"HashSha",
"alg_hash_raw":2,
"alg_auth":"AuthPreSharedKey",
"alg_auth_raw":1,
"alg_dh":"GroupDefault768BitModp",
"alg_dh_raw":1,
"sa_life_type":"LifeTypeSeconds",
"sa_life_type_raw":1,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":65535
},
{
"alg_enc":"EncDesCbc",
"alg_enc_raw":1,
"alg_hash":"HashMd5",
"alg_hash_raw":1,
"alg_auth":"AuthPreSharedKey",
"alg_auth_raw":1,
"alg_dh":"GroupDefault768BitModp",
"alg_dh_raw":1,
"sa_life_type":"LifeTypeSeconds",
"sa_life_type_raw":1,
"sa_life_duration":"Unknown",
"sa_life_duration_raw":65535
}
]
}
}
}
}
}
Files
Updated by Victor Julien 19 days ago
- Target version changed from 8.0.2 to 9.0.0-beta1
Updated by Jason Ish 19 days ago
- File generated.pcap generated.pcap added
- Status changed from New to Assigned
- Assignee changed from OISF Dev to Jason Ish
I've created a PCAP that can reproduce this. Will assign to myself to fix.
Updated by Jason Ish 15 days ago
- Status changed from Assigned to In Progress
For 9.0 it probably makes sense to use a more suitable JSON layout, for example:
"proposals": [
{ "type": "alg_enc", "value": "EncAesCbc", raw: 7 },
{ "type": "sa_life_duration", "value": "Unknown", "raw": 256 },
{ "type": "sa_life_duration", "value": "Unknown", "raw": 65536 }
]
Obviously that is a breaking change for 7 and 8. I see 2 options to keep compatiblity:
- Drop subsequent values that have already been logged (loss of fidelity)
- Add an index to duplicates, for example:
28 │ "encrypted_payloads": false,
29 │ "client": {
30 │ "proposals": [
31 │ {
32 │ "alg_enc": "EncAesCbc",
33 │ "alg_enc_raw": 7,
34 │ "sa_key_length": "Unknown",
35 │ "sa_key_length_raw": 128,
36 │ "alg_hash": "HashSha",
37 │ "alg_hash_raw": 2,
38 │ "sa_life_duration": "Unknown",
39 │ "sa_life_duration_raw": 86400,
40 │ "alg_dh": "GroupAlternate1024BitModpGroup",
41 │ "alg_dh_raw": 2,
42 │ "alg_auth": "AuthPreSharedKey",
43 │ "alg_auth_raw": 1,
44 │ "sa_life_type": "LifeTypeSeconds",
45 │ "sa_life_type_raw": 1,
46 │ "sa_life_duration_1": "Unknown",
47 │ "sa_life_duration_1_raw": 65535
48 │ }
49 │ ]
Any other ideas?
Updated by Jason Ish 13 days ago
- Status changed from In Progress to In Review
PR: https://github.com/OISF/suricata/pull/13907
Discussion of logging changes in the upgrade guide:
https://github.com/OISF/suricata/pull/13907/files#diff-7740e8ee6bb2915c5c4513b5ef5391cfac1704ec81d81206e36910f1f5733a88
Updated by Jason Ish about 22 hours ago
- Status changed from In Review to Resolved