Opened 5 years ago

Closed 4 years ago

#1318 closed enhancement (fixed)

Extending SAM to Allow Creation of More Secure Destinations

Reported by: ExtraBattery Owned by: zzz
Priority: minor Milestone:
Component: apps/SAM Version: 0.9.13
Keywords: docs review-needed Cc:
Parent Tickets: #1574 Sensitive: no

Description

It would be great if SAM was extended so that it allows applications to make use of the new destination types introduced with I2P 0.9.12. For the extension, the SAM version could be slightly changed from 3.0 to 3.1, so that the client application is always aware of the capabilities of the bridge. The full v3.0 feature-set should be retained in v3.1.

Proposed extension:

In a "SESSION CREATE" message, if the key-value pair "DESTINATION=TRANSIENT" is used, an *optional* "DESTSIGTYPE" key is allowed, which can take any of the following seven values (where DSA_SHA1_160 is the default):

DSA_SHA1_160
ECDSA_SHA2_256
ECDSA_SHA2_384
ECDSA_SHA2_521
RSA_SHA2_2048
RSA_SHA2_3072
RSA_SHA2_4096

Alternatively the signature type code may be used as value: 0-6

First example:

SESSION CREATE STYLE=STREAM DESTINATION=TRANSIENT DESTSIGTYPE=ECDSA_SHA2_521 ID=Test

Second example (using the type code):

SESSION CREATE STYLE=STREAM DESTINATION=TRANSIENT DESTSIGTYPE=3 ID=Test

Subtickets

Attachments (3)

SAM 3.1.zip (21.2 KB) - added by ExtraBattery 5 years ago.
Patch
SAM 3.1 rv3.zip (20.4 KB) - added by ExtraBattery 5 years ago.
SAM 3.1 rv3
Checks.txt (504 bytes) - added by ExtraBattery 5 years ago.
Updated payload size checks for non-repliable and repliable datagrams

Download all attachments as: .zip

Change History (32)

comment:1 Changed 5 years ago by ExtraBattery

I've made the necessary changes based on code of I2P 0.9.13-0 and attached them (as code and diff files). I would really appreciate it if someone took half an hour and committed it. I didn't know the maximum byte size of the new destinations (including the private portion), so the exact number should be inserted into the code (there's a comment explaining it).

I didn't test my changes, because I don't have an I2P build environment. I will test if there's an experimental I2P release that includes it. I also offer to update the manual when this is committed.

SUPPLEMENT:

I just updated my patch. The new revision of the patch also allows the creation of more secure destinations with the DEST GENERATE message. In addition it implements the feature requested in ticket #683.

The diffs are still based on comparisons with 0.9.13-0.

Last edited 5 years ago by ExtraBattery (previous) (diff)

Changed 5 years ago by ExtraBattery

Attachment: SAM 3.1.zip added

Patch

comment:2 Changed 5 years ago by zzz

  • Your patch doesn't even compile. (EDIT: sorry, missed the statement above that you didn't test.) An untested patch isn't very helpful, unfortunately. Testing is most of the work.
  • As I commented in #683 I'm not in favor of supporting that feature, unless somebody comes up with a good reason
  • Why do you not use the SigType? enum names and the helper methods in there to resolve them? Renaming them and putting the new strings in an array is bad. Just use the SigType? methods and you support everything that the core lib does and don't need to track core changes.
  • Agreed that the version handling in SAM needs improvement. Getting rid of floats is a good first step. Our net.i2p.util.VersionComparator? class may be helpful here.
Last edited 5 years ago by zzz (previous) (diff)

comment:3 Changed 5 years ago by zzz

  • Also, all the reindenting and other whitespace changes (particularly in the handlers) makes it almost impossible to review the diff. If you'd like us to carefully review and comment on the proposed change, please resubmit it without the whitespace changes. The diff is currently 2161 lines.
  • But unless you can compile and test your submission, it won't be very high priority. Can't you test it?

comment:4 Changed 5 years ago by ExtraBattery

Thanks for looking into this. I will change whatever is needed. The general idea is what matters to me.

  • For my patch to work, one needs at least add "import net.i2p.crypto.SigType?;" to "SAMUtils.java", as I later realized.
  • I will try to come up with a good reasons for #683.
  • I agree that using an enum is a good idea. I just felt that the SigType? declaration is so bloated, that an int is faster. But I'm fine with using SigType? throughout.
  • I included diffs that are much shorter than 2161 lines. Aren't they of any use? I used NetBeans? IDE 8.0 to create diffs and it seems to have ignored whitespace formatting changes when generating diffs. I suppose you are using Eclipse and maybe it works different.

Btw: You could run a tool over the entire code base regularly to convert tabs into 4 (or whatever your desired number is) spaces and that removes any trailing spaces (do you need them?). It would make things easier (for me at least).

  • I must see if I get an I2P build environment somehow…
Last edited 5 years ago by ExtraBattery (previous) (diff)

comment:5 Changed 5 years ago by zzz

I glanced through the diff files, but then I copied over the full files and ran a mtn diff on them. Only later did I realize that the diff files didn't match the full files.

The compile error I found was an extra paren somewhere…

I don't understand the 'bloated' comment. Whatever you think of the SigType? code, it's all done. SigType?.parseSigType(String foo) does what you need.

comment:6 Changed 5 years ago by ExtraBattery

I completely rewrote the patch to take your suggestions into account. The new patch is not online yet, however it's already very stable. I now do lots of testing. I will release it when testing is through.

Forget what I said about SigType? being bloated. I was just not accustomed to enums being objects.

Changed 5 years ago by ExtraBattery

Attachment: SAM 3.1 rv3.zip added

SAM 3.1 rv3

comment:7 Changed 5 years ago by ExtraBattery

The new patch ("SAM 3.1 rv3.zip" is now online).

During testing one problem cropped up that I couldn't handle myself:

There is a routine called checkPrivateDestination() in SAMUtils.java. It was buggy in that it didn't check if the entire buffer was read before letting the input pass, so any nonsense was allowed at the end of a destination. Consequently I added a check to see if everything was read. *BUT* this change caused the CERTIFICATE_TYPE_KEY destinations to no longer 'pass' the test, as the 'check' leaves some of their trailing data unread. So I restored the original buggy state, to allow CERTIFICATE_TYPE_KEY dests again. I leave it to your capable hands to update the check so that is able to verify the new dests and at the same time rejects any invalid data. It would be good to have routine outside of SAM for this purpose, so that SAM doesn't have to get updated whenever a new destination type is introduced.

Other than that everything is fine. The patch is tested and ready. All changes are based on git hub files from 24-June-2014 (I downloaded "i2p.i2p-master.zip" on 25-June-2014). Six code files were changed.

  • I left the #683 stuff away this time. The only thing I did that is not related to #1318 is related to #1325: I included a check for key-value pairs, so that keys that are an empty string are no longer allowed, since they are supposed to be illegal.
  • I based everything on SigType?.parseSigType() this time, I just didn't allow lowercase strings for the identifiers, since that would be untypical for SAM and ambiguous. I also changed the name of the newly introduced property from the proposed "DESTSIGTYPE" to "SIGNATURE_TYPE", which looks more like SAM (also "DESTINATION" occurs contextual already).
  • I couldn't find a use for net.i2p.util.VersionComparator?. I rewrote the version number handling, so that it's now more integer based and strings are only used where really necessary (and floats aren't used anymore).
  • I didn't change white spaces of otherwise unmodified lines and tried to code in your style.
  • During testing I detected that the condition (props == null) that a lot of handlers check for will never be met. So I changed these checks to props.isEmpty() as was obviously meant.
  • The patch is well tested. Applications that use the feature-set of SAM 3.0 or earlier will not experience behavioral differences for legal input.
Last edited 5 years ago by ExtraBattery (previous) (diff)

comment:8 Changed 5 years ago by zzz

Owner: changed from mkvore to zzz
Status: newaccepted

Great.

I haven't studied the #1325 issue yet.

I've also been working on the versioning stuff using VersionComparator?. Haven't tested yet. Will need to be careful we don't break existing apps. Will have to review yours and mine and see which looks better.

I'll probably pick out specific parts of your patch rather than check in the whole thing at once.

I have a fix for checkPrivateDestination, not yet tested.

comment:9 Changed 5 years ago by ExtraBattery

I successfully fuzz tested my patched version handling with the following client code. The first param is the min ver, the second the max ver. The third param is the expected SAM reply, null if "RESULT=NOVERSION" is expected.

(DoVersionTestCore?("1.0", "1.0", "1.0") &&
DoVersionTestCore?("2.0", "2.0", "2.0") &&
DoVersionTestCore?("3.0", "3.0", "3.0") &&
DoVersionTestCore?("3.1", "3.1", "3.1") &&
DoVersionTestCore?("1.3", "1.4", null) &&
DoVersionTestCore?("3.0", "3.1", "3.1") &&
DoVersionTestCore?("3.0", "3.2", "3.1") &&
DoVersionTestCore?("1.0", "3.0", "3.0") &&
DoVersionTestCore?("3.2", "3.0", null) &&
DoVersionTestCore?("3.2", "3.2", null) &&
DoVersionTestCore?("1.2", "1.3", null) &&
DoVersionTestCore?("0.0", "10.9", null) &&
DoVersionTestCore?("0.0", "9.10", null) &&
DoVersionTestCore?("0.0", "9.9", "3.1") &&
DoVersionTestCore?("-1.1", "3.2", null) &&
DoVersionTestCore?("1.-1", "3.2", null) &&
DoVersionTestCore?("1", "3.2", null) &&
DoVersionTestCore?("0x3.0x0", "3.2", null))

I also tested my other stuff with such techniques and a real life SAM application, running on the patched SAM v3.0 and v3.1.

If you put the checkPrivateDestination() update online somehow, I will test it.

What is the supposed tab vs. space relation of the I2P code? 4 spaces are one tab? Because NetBeans? (which I used) uses 8 spaces by default, with seems to be too much.

Last edited 5 years ago by ExtraBattery (previous) (diff)

comment:10 Changed 5 years ago by ExtraBattery

I see the new checkPrivateDestination(). It's clearly better than the original. However in my tests it didn't work flawlessly. I generated a CERTIFICATE_TYPE_NULL private key and appended an 'h' to the base64, then sent it in. It got accepted. Even with the available() == 0 check. If you append just three base64 chars or less, it gets accepted. Something seems to read them and then throws them away.

Making the routine catch IOExceptions is a smart move (since readBytes() requires it).

May the reference passed to the constructor ByteArrayInputStream?(byte[] buf) be null? Because if not, this is another potential error. It might explain why SAM doesn't answer when I use "DESTINATION=%$&".

You could also add a check for input that is longer than 516 chars to the start of the routine in order to discard some forms of invalid data quicker.

I realized that the checkPrivateDestination() code is similar to the instance routine I2PSessionImpl.readDestination(). Shouldn't I2PSessionImpl get a static variant of that routine that SAM and BOB just call instead of duplicating the code?

In SAMv3Handler.java you don't have to check the destination if it was just created ("DESTINATION=TRANSIENT"). But that's already part of my patch.

Last edited 5 years ago by ExtraBattery (previous) (diff)

comment:11 Changed 5 years ago by zzz

Milestone: 0.9.14
Status: acceptedtesting

SIGNATURE_TYPE support checked in as 0.9.13-10. Version is bumped to 3.1.

I believe this checkin also address everything in comment 10 above, except the suggestion in the next-to-last paragraph about adding a method for checking destinations + private keys. That may be worth doing but I haven't looked into it yet.

re: tabs/spaces question in comment 9 above, almost all of I2P is 4 spaces. SAM however is a jumbled disaster, seems to vary by file or line.

comment:12 Changed 5 years ago by zzz

… and website specs updated to match

comment:13 Changed 5 years ago by ExtraBattery

Thanks for checking it in. The core feature is working. Some remarks on remaining weaknesses:

  • The problem described in the first paragraph of comment 10 has not been dealt with yet: You can still append an up to three chars long random sequence (in base64) to a private key string and it's accepted by checkPrivateDestination().
  • SAMv1Handler.java and SAMv2Handler.java still contain "i2cpProps != null" checks that have no effect.
  • What I don't like is that you left away my checks for SAM v3.1. So one can use the SIGNATURE_TYPE property in v3.0. While this enhances SAM v3.0, there is no guarantee that the SAM v3.0 one uses has the SIGNATURE_TYPE capabilities. So the feature-set of v3.0 has now become somewhat vague. Of course the client application should use v3.1 when using SIGNATURE_TYPE, but checking for v3.1 in SAM is cleaner IMHO.
  • I think my version handling was more correct, more cleanly implemented, faster and more easily extendable (by simply adding a short to an array). Now you have to add three lines of string checking code for each new version. Did you mean to allow input like "MIN=0.0 MAX=9;1000"? Because you have. You also allowed things like "MIN=0.-1 MAX=9.0". You even allowed "MIN=0x3.0x0 MAX=9.0". Proposal: Use my version handling. It's tested and works.

Still thank you for getting this feature on the road! It's a great step to bring more security to applications. As it will take a long time until it's adopted by older applications in any case, it's important to enable things in SAM early on.

Last edited 5 years ago by ExtraBattery (previous) (diff)

comment:14 in reply to:  13 Changed 5 years ago by zzz

Replying to ExtraBattery:

Thanks for reviewing and testing.

Thanks for checking it in. The core feature is working. Some remarks on remaining weaknesses:

  • The problem described in the first paragraph of comment 10 has not been dealt with yet: You can still append an up to three chars long random sequence (in base64) to a private key string and it's accepted by checkPrivateDestination().

Hmm. I think that problem must lie in Base64.decode(). Will have to investigate further. Not sure if we can fix it without breaking other things.

  • SAMv1Handler.java and SAMv2Handler.java still contain "i2cpProps != null" checks that have no effect.

Looks to me like i2cpProps can be null. See SAMBridge constructor - Options.opts can be null.

  • What I don't like is that you left away my checks for SAM v3.1. So one can use the SIGNATURE_TYPE property in v3.0. While this enhances SAM v3.0, there is no guarantee that the SAM v3.0 one uses has the SIGNATURE_TYPE capabilities. So the feature-set of v3.0 has now become somewhat vague. Of course the client application should use v3.1 when using SIGNATURE_TYPE, but checking for v3.1 in SAM is cleaner IMHO.

That was intentional. There's no reason to deny SIGNATURE_TYPE no matter what version was requested. If the client needs to know if it is available, it must request a MAX=3.1 or higher. If it doesn't, we continue to return 3.0. But there's no reason to hide a feature that's backwards compatible. That's not time or code worth writing, in my opinion. Basic principle -be lenient in what you accept.

  • I think my version handling was more correct, more cleanly implemented, faster and more easily extendable (by simply adding a short to an array). Now you have to add three lines of string checking code for each new version.

Well, we'll have to disagree then. I thought it was cleaner to use VersionComparator?, and to not change everything to shorts. Not that there's anything wrong with the way you did it. I just made a decision. I used at least half of your patch as-is, and used a lot of the rest as a reference.

Did you mean to allow input like "MIN=0.0 MAX=9;1000"? Because you have. You also allowed things like "MIN=0.-1 MAX=9.0". You even allowed "MIN=0x3.0x0 MAX=9.0". Proposal: Use my version handling. It's tested and works.

VersionComparator? is a general-purpose utility that's designed to be lenient in what it accepts. It ignores unknown characters. That means some things like your examples above will be allowed. I'm not worried about it. There's no requirement for strict parsing that rejects every possible bad input. It's for one program to talk to another; not a CLI for manual use. Again - being lenient in what we accept.

Still thank you for getting this feature on the road! It's a great step to bring more security to applications. As it will take a long time until it's adopted by older applications in any case, it's important to enable things in SAM early on.

comment:15 Changed 5 years ago by zzz

As it will take a long time until it's adopted by older applications in any case, it's important to enable things in SAM early on.

At some point, maybe later this year, the default will change. Client apps will not need to be modified, unless they made bad assumptions about dest and key lengths.

comment:16 Changed 5 years ago by ExtraBattery

Agreed, I was wrong about i2cpProps.

About that first paragraph (of comment 10) problem: I suppose your base64 implementation reads but ignores un-padded trailing chars that do not form a complete chunk (four chars).

So you seem to have two options:

  • Change your base64 implementation so that it returns null for such input. I would do that. Of course you don't know what breaks then, but that input is illegal. At the moment you can also successfully connect to a *public* destination with appended (less than four base64 chars) nonsense through SAM.
  • Add "((dest.length() % 4) != 0) return false;" to the first line of checkPrivateDestination(). You then still need to add similar code to the instance routine Destination.fromBase64(String foo) to deal with public destinations (or you limit this change to getDest() and checkDestination() of SAM).

Do you really believe that your version checking is equally correct, equally fast and as easily extendable as my approach? I think you are way too lenient toward that version parsing stuff. Come on: "0.-1" and "0x3.0x0". If you don't alert an application developer to the error he/she makes, they go unnoticed and you effectively promote bad software. Of course you can blame it on the application developer, but all of I2P suffers.

You actually want to change the default from SigType?.DSA_SHA1 to something else?? Good to know. Then you should use a "static final" field for the default instead of hardcoding SigType?.DSA_SHA1 in different places, to make later changes easier.

Last edited 5 years ago by ExtraBattery (previous) (diff)

comment:17 Changed 5 years ago by zzz

It looks like Base64 ignores chars after an '='. If it didn't end with an '=' it will barf I think. Untested. You can play with it using java -cp i2p.jar net.i2p.data.Base64 . I see some stuff in there I could clean up.

re: version, I appreciate your passion for what apparently is the first Java code you've written? It's really good. Is the version part better than mine? sure. Is it worth the added code? no. Is it worth any more time than I've already spent on this very-low-priority ticket? no. "promote bad software" and "all of I2P suffers" is really hyperbolic. Let's work on some other tickets that matter.

re: static final, sure, will address that when the time comes.

comment:18 Changed 5 years ago by zzz

Yeah, Base64 decode really sucks. Working on it now.

comment:19 Changed 5 years ago by ExtraBattery

Okay, I won't talk about that version stuff anymore.

These base64 changes will improve anything that relies on DataStructure?.fromBase64().

comment:20 Changed 5 years ago by zzz

in 13bf275f1e0f028303643bb63930b6ee63840af1 0.9.13-13

  • Catch numerous decoding errors that were previously misdecoded
  • Improve decoding efficiency, reduce copies
  • encode(String) now uses UTF-8 encoding
  • decode() now accepts short strings without trailing '='
  • whitespace in decode will now cause an error, was previously ignored
  • Cleanups

comment:21 Changed 5 years ago by zzz

Resolution: fixed
Status: testingclosed

The Base 64 changes broke a unit test (whitespace, fixed) and i2p-bote (bad length, fixed) so the new strict decoding is finding problems. That's good.

The issue in the OP is fixed, as reported in comment 13 above, so closing this ticket.

comment:22 Changed 5 years ago by ExtraBattery

Sounds great!

comment:23 Changed 5 years ago by ExtraBattery

I just realized that there is a function - checkDatagramSize() - in "SAMv1Handler.java" that checks the size of a repliable datagram before sending it. It requires the payload to be within the range of 1 to 31744 bytes. I think this is not correct anymore, since a RSA 4096-bit destination is 388 bytes longer than a DSA-160 destination and has a signature that is not 2x20 bytes long, but supposedly 512 bytes long.

So I assume the upper limit should be updated in some form. The entire check could be made SigType? dependent and centralized inside the router, which would i.e. have an array with the limits for repliable datagrams by SigType? for this purpose.

It would also be good conduct to write the repliable datagram limits by destination type into the documentation.

https://geti2p.net/en/docs/api/datagrams

comment:24 Changed 5 years ago by zzz

Milestone: 0.9.140.9.15
Resolution: fixed
Status: closedreopened

comment:25 Changed 5 years ago by zzz

Reopened. When there's more to do, please either reopen the closed ticket or open a new ticket, otherwise it will get forgotten.

Changed 5 years ago by ExtraBattery

Attachment: Checks.txt added

Updated payload size checks for non-repliable and repliable datagrams

comment:26 Changed 5 years ago by ExtraBattery

I've written a modified version ("Checks.txt​") of the size checks that should be able to produce the correct results. The check-routines should not be in "SAMv1Handler.java" I believe, but in "SAMMessageSession.java", where they can be used by code other than the SAM v1 handler too. For example "SAMDatagramSession.java" currently has its own constant "DGRAM_SIZE_MAX" for the maximum datagram size. This value should be obliterated. All code that does datagram payload size checking should use the same set of routines.

Of course code that performs the size check for repliable datagrams must pass the SigType? of the origin destination for the check to work. Something I haven't included in my patch. I feel it would be wise to store the SigType? of the local destination in the SAM session object whenever a session is created, so that it's readily available.

comment:27 Changed 5 years ago by str4d

Keywords: docs review-needed added
Milestone: 0.9.15

comment:28 Changed 4 years ago by zzz

Parent Tickets: 1574

comment:29 Changed 4 years ago by zzz

Resolution: fixed
Status: reopenedclosed

Not fixing 0-length datagrams. See #1335

We aren't using RSA destinations so the long-length checks are good enough for now.

The rest of the ticket is already fixed, re-closing.

Note: See TracTickets for help on using tickets.