Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for IPv6 #84

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

BitBlit88
Copy link

This repo fixes issues with IPv6.
There were errors like:

IPv6 is not supported under this link-layer type

if the interface has only an IPv6 address but no IPv4 address.

Updated copyright notes
Fixed issue with uninitialized ip6 pointer in case of packet type DLT_LINUX_SLL and DLT_RAW
Removal of whitespaces
Bump version to 8.0 (due to major IPv6 feature)
Update copyright notes
Fixed handling of v6 pcap filter rule recreation in case of one time sequences
… which in case set will remove the generated filter strings. That reduces the allocated memory, but in case of one-time door sequences consumed requires all filters to be recreated in generate_pcap_filters(). Also take care of proper free() handling.
@TDFKAOlli
Copy link
Contributor

TDFKAOlli commented Mar 20, 2022

@BitBlit88 , not sure if that works. My version derived a bit due to different work on memory safety issues (#74) which were merged here in this repo and what I did on my fork. I had questions about the last changes but they where merged before those where clarifed. @dimkr used lint or another code checker to find the memory issues in the code and I always dreamed of making the core code unit testable so to be able to really test all situations and thereby find all leaks.
I think if you want to take stuff from my fork you need to cherry pick.

@dimkr
Copy link
Contributor

dimkr commented Mar 20, 2022

@TDFKAOlli I used Valgrind and -fsanitize=address. I can help with conflict resolution and code review, if needed.

@TDFKAOlli
Copy link
Contributor

TDFKAOlli commented Mar 22, 2022

@dimkr Thanks. I checked through the conflicts and resolved them, creating a new pull request on my master. In the end the changes on this master seems logic and I couldn't see issues anymore (and I don't know which ones I have seen in the past 😄 ).

@BitBlit88 So the conflicts are at least resolved. I have not tested the result yet, but will try to do so with the branch created. Not sure if you can do additional commits here for your "any" feature.

@jvinet I have adapted the copyright notes in my branch just to mention the contributers. That might not been right, its your code base, so you might want to change this back to what you had before.

EDIT: Damn, seems somehow some bugs where fetched. The pcap-filter "tcp-psh" is wrong it has to be "tcp-push". That was right on my branch, now it is broken in the new branch... 😢

src/knockd.c Outdated
if (tmp->ifa_addr && tmp->ifa_addr->sa_family == AF_INET) {
struct sockaddr_in *addr_in = (struct sockaddr_in *)tmp->ifa_addr;
if (strcmp(iface,tmp->ifa_name) == 0 ) {
strncpy(buf, inet_ntoa(addr_in->sin_addr), bufsize-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just return inet_ntop(tmp->ifa_addr->sa_family, buf, tmp->ifa_addr, bufsize), you don't need to copy a string

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't find any usage of get_ip() anymore. Seems to be a leftover from an old implementation. I'll remove.

src/knockd.c Outdated
free(door->stop_command);
if(door->one_time_sequences_fd) {
if(door->start_command) {
free(door->start_command);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

free(NULL) is safe, you can easily reduce the diff

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

src/knockd.c Outdated
@@ -795,6 +797,10 @@ int parseconfig(char *configfile)
fprintf(stderr, "error: section '%s' has an empty knock sequence\n", door->name);
return(1);
}
if(door->start_command == NULL && door->start_command6 == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if !start_command[0]? Is that OK?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, should also check for strlen() as also done in process_attempt().

Copy link
Contributor

@dimkr dimkr Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you only want to check if a string is empty, x[0] is more efficient than strlen(x) == 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer readabiliy over efficiency, especially as this is just done once at startup when reading in the commands.

@TDFKAOlli
Copy link
Contributor

So, now cleaned up a bit and also built for my OpenWrt router. Seems all fine.
tcp-psh vs. tcp-push seemed to be my commit from 2019, I guess based on tcpdump information. I have tested with tcp-push and that seems to compile fine.

@dimkr Thanks, will also have a look at your comments.

@TDFKAOlli
Copy link
Contributor

@dimkr Should be fine now.
@BitBlit88 I have added the "any" interface support as described in the issue discussion. It doesn't seem to work on my OpenWrt. I can configure the 'any' interface, but the packets are not captured. Have to check what is the problem there, because that would be useful for me too. 😏

@TDFKAOlli
Copy link
Contributor

TDFKAOlli commented Mar 29, 2022

Fixed the problem that 'any' doesn't capture on OpenWRT. That was my fault in adding the check for 'any'. 😄 Now there is another problem with setting the any interface... I do get each packet twice. Apparently from the same IP source and dest address as the logs state. That breaks the attempts as duplicates are currently not allowed. Have to check where that is coming from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants