SSU: review handling of rekey option

[EinMByte]

Causes a segmentation fault due to use of uninitialized memory.
Usual logging output you will see:

DBG     SSU Packet has rekey data
DBG     SSU Packet has extended options
DBG     Session request received
ERR     Couldn't create shared key
Program received signal SIGSEGV, Segmentation fault.

The cause seems to be that even though the rekey flag is set (it's currently unimplemented, so why is it set?), there is no rekey data. The result is that (after a while) the implementation tries to encrypt uninitialized data, leading to a segfault.

Part of the solution would be to check if we received enough data, which would avoid the crash

[anonimal]

Related to #101 #16. Referencing #104.

[anonimal]

17:10:25        +zzz | Ein, that looks like complete garbage. bits 0 1nd 1 are reserved. extended options is bit 2. session created should be 368+ bytes. this is after mac check?                                               │
17:10:53        +zzz | ^^ had that buffered a couple of days waiting for him to reappear. posting now so it isn't lost

[anonimal]

Hi @EinMByte. Any updates re: this issue? I'd like to help but would prefer to work within our development branch instead of forking your fork - so any updates would be helpful before I dive into this.

[EinMByte]

@anonimal If you want a quick fix, you can just ignore the rekey option (like i2pd does). I haven't managed to find any time to work on the SSU refactoring lately, which in my opinion should eventually lead to the solution of this issue.

[anonimal]

If we do that, AFAICT, we'll have to revert d2c0908 and 607e898 (maybe more) because of the spec requirement for .24 - but I haven't looked into this further to say that with complete confidence. I would personally rather spend the time resolving this without ignoring rekey but that means I'll have to shift focus to SSU - which I'm fine with doing (I think I had planned to do this at one of our meetings after #149 was resolved).

I'll stay tuned for questions or findings.

[EinMByte]

@anonimal I don't think rekeying is supported by any router. Extended options are supported, but the rekey option itself doesn't do anything anyway. With "ignoring" I really meant dropping packets that have the option (and not try to parse/process them anyway).

[anonimal]

Ah, I see now, I wasn't paying attention re: rekey vs. extended options (and I should have reviewed the spec again before commenting - my mistake). Thanks for clearing that up.

[anonimal]

JFTR, here's a backtrace.

[anonimal]

The result is that (after a while) the implementation tries to encrypt uninitialized data, leading to a segfault.

@EinMByte as noted in 1876b42, initializing rekey data (apparently) has no effect. Can you confirm? Also, if we simply want to drop packets when rekey is set, we can throw before packet parsing without any further issue (afaict).

Part of the solution would be to check if we received enough data, which would avoid the crash

As noted in code, you are consuming the correct length of rekey (and header) data (afaict). What other solution did you have in mind?

Re-labeling since the segfault is no longer an issue.

[EinMByte]

@anonimal AFAIK, the current behavior is to ignore the rekey option. It may be better to reject it.
Both would "work", though.

[anonimal]

NOTICE: THIS ISSUE HAS BEEN MOVED TO GitLab. Please continue the discussion there. See #1013 for details.