craigerl / ygate

Turn your Yaesu APRS radio into an igate. By KM6LYW.
28 stars 9 forks source link

ygate has numerous igate bugs, corrupts packets #1

Closed hessu closed 5 years ago

hessu commented 6 years ago

ygate seems to modify packet content by stripping whitespace, for one. Please do not deploy this igate on the APRS-IS as it is before implenting a more complete igate, and before ensuring it is binary clean.

Before fixing it, please instruct users in the README that it should not be used live, as it causes problems to the APRS network.

Some more documentation:

https://github.com/hessu/aprsc/blob/master/doc/IGATE-HINTS.md http://blog.aprs.fi/2017/05/important-hints-to-igate-developers.html http://www.aprs-is.net/IGateDetails.aspx

Thank you!

craigerl commented 6 years ago

Thanks for the peer review for the Ygate code.  The aprs spec is hard to lock down, and my browser "visited link" color shows I've seen two of the links you posted.  I'll check the other two, thank you!

In my tests, Ygate corrupted no packets, and I compared roughly 100 or so, in fact, just to setup a positive-positive test, I manually corrupted a packets, and Ygate suddenly started getting tons of credit for the (artificially) unique packets, confirming that Ygate was gating packets just like every other nearby igate, only slower.   In any event  ygates output was the same in a hexeditor, compared to the nearby (direwolf) igates which won the race.

I've not seen NOGATE/RFONLY in the wild, nor in a spec, so I'll certainly filter for those.

libtelnet seemed sufficient to me and the code meets all requirements using python's libtelnet.  I'll look into raw sockets too.  That was on my todo list, thanks.

"TCP" routing actually shows up in the comment section of the bizarre aprs "third party" packet, and the spec indicates it's critical to drop all internet-sourced packets, so I drop frames on the side of caution even if the sender is simply talking about tcp networking :).  I'll lock it down better with a third-party- packet exception case.

I don't know what trim() is, which you referred to in another forum, but in any event strip() is required to remove all the random line-feeds yaesu radios inject into the aprs packet and attempt to put them in an nema9 format.   The strips remove these line feeds, which was the point of the software, and retain the original, un-corrupted routing and text.

hessu commented 6 years ago

strip() removes all whitespace, including spaces and tabulator characters from the end (and beginning) of the string. When a packet has spaces in the end (for example, Yaesu VX-8 mic-e type code which happens to be a space... or when someone accidentally types a space character in the end of a comment), your igate removes those spaces, and creates a modified duplicate packet (i.e. corrupts it).

Look for the first CR or LF in the packet data, cut it there, but please don't touch other whitespace.

Using a telnet lib may well cause confusion, as the server is not a telnet server, and will not escape binary sequences which may be handled as telnet negotiation by your telnet client.

craigerl commented 6 years ago

Agreed - strip is strictly limited to CR/LF now (of which Yaesu's inject many). As a test, I put a delay in the client, allowing all other igates to gate traffic ahead of me. Luckily, none of my packets are showing up since they're exact duplicates of other reports (indicating mine are not corrupt, not unique). I'll keep watching. Thanks. I'll look into socket io next.

hessu commented 6 years ago

Luckily, none of my packets are showing up since they're exact duplicates of other reports (indicating mine are not corrupt, not unique)

That is not an indication of them not being corrupted. It just happens because this kind of corruption is unfortunately quite common and I've had to write special code in the aprsc server to try and catch these.

https://github.com/hessu/aprsc/blob/master/src/dupecheck.c#L382 ... https://github.com/hessu/aprsc/blob/master/src/dupecheck.c#L414

If a corrupted packet (one with spaces trimmed) will come in to the server later than an uncorrupted one, it will be dropped as a duplicate. If the corrupted version comes first (if your igate was fast...) and an uncorrupted one appears later, the uncorrupted one will not be dropped, because, well, it is preferred to the corrupted onne.

ChrisK9EQ commented 5 years ago

Just a general comment. Good programming practices require that any function/service receiving data completely validate that data. If validation doesn't pass, drop it. Not doing this is the source of a lot of security issues. You should be able to throw completely random data of any length at an incoming port and not cause problems for the server.

craigerl commented 5 years ago

agreed, this is why we use very specific regular expressions to filter input.

ghost commented 5 years ago

Anyone still doing any development of this script? Has there been any developments as far as fixing the above problems?

craigerl commented 5 years ago

I'm happy with it, and it passes my QA. I updated the script to strip only carriage returns (not whitespace), consistent with the spec and what we expect from the yaesu NMEA9 sentences. I implemented the RFONLY/NOGATE items, and don't gate anything sourced from the Internet as well. I noticed there's an aprs python library out there -- at some point I'd like to use that. I appreciate feedback and bug reports.