RestComm / jss7

RestComm Java SS7 Stack and Services
http://www.restcomm.com/
GNU Affero General Public License v3.0
177 stars 218 forks source link

Wrong length check in UserDataHeaderImpl.java #275

Closed smarek closed 6 years ago

smarek commented 6 years ago

Check in getApplicationPortAddressing16BitAddress() method of UserDataHeaderImpl is wrong, 16bit application port by docs ETSI TS 100 901 V7.5.0 (2001-12) section 9.2.3.24.4 is 4 octets (2 for destination and 2 for originator port)

In parser implementation this check is actually implemented correctly (see ApplicationPortAddressing16BitAddressImpl.java#L53 ), and these kind of checks might be eliminated from UserDataHeaderImpl.java if correct input to parser is checked on field-level.

Below proposed trivial fix diff against current master at 91be33cd42f03ba030d6e2381b91a96f76354269

diff --git a/map/map-impl/src/main/java/org/mobicents/protocols/ss7/map/smstpdu/UserDataHeaderImpl.java b/map/map-impl/src/main/java/org/mobicents/protocols/ss7/map/smstpdu/UserDataHeaderImpl.java
index aa750c8f3..f49677366 100644
--- a/map/map-impl/src/main/java/org/mobicents/protocols/ss7/map/smstpdu/UserDataHeaderImpl.java
+++ b/map/map-impl/src/main/java/org/mobicents/protocols/ss7/map/smstpdu/UserDataHeaderImpl.java
@@ -143,7 +143,7 @@ public class UserDataHeaderImpl implements UserDataHeader {
     @Override
     public ApplicationPortAddressing16BitAddress getApplicationPortAddressing16BitAddress() {
         byte[] buf = this.data.get(_InformationElementIdentifier_ApplicationPortAddressingScheme16BitAddress);
-        if (buf != null && buf.length == 1)
+        if (buf != null && buf.length == 4)
             return new ApplicationPortAddressing16BitAddressImpl(buf);
         else
             return null;
vetss commented 6 years ago

Hello @smarek

thanks for your feedback. Your note is correct.

May I ask you to read THE TELESTAX OPEN SOURCE PLAYBOOK: https://telestax.com/open-source/#Contribute and sign Contributor License Agreement

smarek commented 6 years ago

I signed CLA, not with github, but with corporate e-mail account, but https://github.com/smarek is listed as my profile, so you should be able to match me against list of signatures.

vetss commented 6 years ago

Thanks @smarek

I have added your fix by this commit https://github.com/RestComm/jss7/commit/4c14755c8073e00a4042aef15f22e7ddd45d8ca7

Thanks for update.

gsaslis commented 6 years ago

@smarek thanks for contributing this! 👍

I've already added your name in our Contributors Hall of Fame 👏 . If you would like to open a PR so that we can merge this in properly, we'd also be happy to do that too. Just let me know ;)

smarek commented 6 years ago

@gsaslis thanks, this one is already fixed by mentioned commit, so i'll directly provide PR next time we find out something to fix. Cheers!

gsaslis commented 6 years ago

awesome!

(fwiw - i was just talking about you submitting a brand new PR with the same fix, so we can ensure the commit bears your name in GitHub. Probably just a minor detail now, but thought I'd check anyway ;) )

Looking forward to your future PRs !