Page MenuHomeVyOS Platform
Create Task
Maniphest T816

ipaddrcheck / libcidr but on IPv6 network validation
Closed, ResolvedPublicBUG
Actions

Description

IPV6 addresses containing a double colon :: two times are treated as valid.

This enables users to configure wrong IPv6 addresses via the VyOS CLI which ises this validator.

ipaddrcheck returns 0 on success

# valid address
root@LR1:~# ipaddrcheck --is-ipv6 2001:db8::1
root@LR1:~# echo $?
0
# invalid address
root@LR1:~# ipaddrcheck --is-ipv6 2001:db8::g
root@LR1:~# echo $?
1
# valid address
root@LR1:~# ipaddrcheck --is-ipv6 2001:db8::1/64
root@LR1:~# echo $?
0
# invalid address (two times '::')
root@LR1:~# ipaddrcheck --is-ipv6 2001::db8::1/64
root@LR1:~# echo $?
0

Details

Difficulty level
Unknown (require assessment)
Version
1.2.0-rolling+201808311253
Why the issue appeared?
Will be filled on close

Event Timeline

c-po assigned this task to dmbaturin.Sep 1 2018, 8:04 AM
c-po triaged this task as High priority.
c-po created this task.
c-po updated the task description. (Show Details)Sep 1 2018, 8:06 AM
masterit added a subscriber: masterit.Sep 15 2018, 8:46 PM
syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc2); removed VyOS 1.2 Crux (VyOS 1.2.0-rc1).Oct 9 2018, 6:42 AM
syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc3); removed VyOS 1.2 Crux (VyOS 1.2.0-rc2).Oct 13 2018, 9:28 AM
EwaldvanGeffen added a subscriber: EwaldvanGeffen.Oct 14 2018, 11:16 PM

regex or for the lazy

ipaddrcheck_functions.c#L104
ipaddrcheck_functions.c#L131

in PR

@dmbaturin

EwaldvanGeffen moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc3) board.Oct 14 2018, 11:46 PM
c-po added a comment.EditedOct 15 2018, 8:21 AM

https://ci.vyos.net/job/ipaddrcheck/build=jessie64devel.vyos.net/5/console

ipaddrcheck_functions.c
ipaddrcheck_functions.c: In function ‘is_ipv6_cidr’:
ipaddrcheck_functions.c:104:23: error: unknown escape sequence: '\.' [-Werror]
     re = pcre_compile("^(((?:(?:(?:[A-F0-9]{1,4}:){6}|(?=(?:[A-F0-9]{0,4}:){0,6}(?:[0-9]{1,3}\.){3}[0-9]{1,3}$)(([0-9A-F]{1,4}:){0,5}|:)((:[0-9A-F]{1,4}){1,5}:|:)|::(?:[A-F0-9]{1,4}:){5})(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])|(?:[A-F0-9]{1,4}:){7}[A-F0-9]{1,4}|(?=(?:[A-F0-9]{0,4}:){0,7}[A-F0-9]{0,4}$)(([0-9A-F]{1,4}:){1,7}|:)((:[0-9A-F]{1,4}){1,7}|:)|(?:[A-F0-9]{1,4}:){7}:|:(:[A-F0-9]{1,4}){7}))(\\/\\d{1,3}))$",
                       ^
ipaddrcheck_functions.c:104:23: error: unknown escape sequence: '\.' [-Werror]
ipaddrcheck_functions.c: In function ‘is_ipv6_single’:
ipaddrcheck_functions.c:131:23: error: unknown escape sequence: '\.' [-Werror]
     re = pcre_compile("^(?:(?:(?:[A-F0-9]{1,4}:){6}|(?=(?:[A-F0-9]{0,4}:){0,6}(?:[0-9]{1,3}\.){3}[0-9]{1,3}$)(([0-9A-F]{1,4}:){0,5}|:)((:[0-9A-F]{1,4}){1,5}:|:)|::(?:[A-F0-9]{1,4}:){5})(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])|(?:[A-F0-9]{1,4}:){7}[A-F0-9]{1,4}|(?=(?:[A-F0-9]{0,4}:){0,7}[A-F0-9]{0,4}$)(([0-9A-F]{1,4}:){1,7}|:)((:[0-9A-F]{1,4}){1,7}|:)|(?:[A-F0-9]{1,4}:){7}:|:(:[A-F0-9]{1,4}){7})$",
                       ^
ipaddrcheck_functions.c:131:23: error: unknown escape sequence: '\.' [-Werror]
cc1: all warnings being treated as errors
Makefile:363: recipe for target 'ipaddrcheck_functions.o' failed
make[3]: *** [ipaddrcheck_functions.o] Error 1
make[3]: Leaving directory '/home/jenkins/workspace/ipaddrcheck/build/jessie64devel.vyos.net/src'
pasik added a subscriber: pasik.Oct 15 2018, 9:52 PM
syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc4); removed VyOS 1.2 Crux (VyOS 1.2.0-rc3).Oct 15 2018, 10:15 PM
syncer moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc4) board.
EwaldvanGeffen added a comment.Oct 16 2018, 7:02 PM

I've redone the patch, it uses a simpler regex because we do not need mixed-mode. The main issue was it didn't validate lowercase. I had to split the host/mask parsing to do the cidr variant properly. I've added tests and it ran succesfully. PR#2

syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc5); removed VyOS 1.2 Crux (VyOS 1.2.0-rc4).Oct 24 2018, 4:40 PM
syncer moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc5) board.
syncer added a subscriber: syncer.Oct 29 2018, 9:59 AM

@c-po can we mark this as resolved?

syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc6); removed VyOS 1.2 Crux (VyOS 1.2.0-rc5).Oct 29 2018, 6:16 PM
syncer moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc6) board.
syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc7); removed VyOS 1.2 Crux (VyOS 1.2.0-rc6).Nov 6 2018, 12:58 AM
syncer moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc7) board.
syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc8); removed VyOS 1.2 Crux (VyOS 1.2.0-rc7).Nov 13 2018, 3:49 AM
syncer moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc8) board.
syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc9); removed VyOS 1.2 Crux (VyOS 1.2.0-rc8).Nov 20 2018, 12:45 PM
syncer moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc9) board.
syncer edited projects, added VyOS 1.2 Crux (VyOS 1.2.0-rc10); removed VyOS 1.2 Crux (VyOS 1.2.0-rc9).Nov 27 2018, 12:04 AM
syncer moved this task from Needs Triage to In Progress on the VyOS 1.2 Crux (VyOS 1.2.0-rc10) board.
dmbaturin closed this task as Resolved.Nov 28 2018, 11:20 PM

Apparently we do not have phabricator integration setup for the ipaddrcheck repo, since it didn't pick this commit up: https://github.com/vyos/ipaddrcheck/commit/21c0775c51da1ca3d4ef6506fca82bce5b334c79

Yes, it's resolved now. I've also added unit tests to prevent this bug from re-appearing.

syncer moved this task from In Progress to Finished on the VyOS 1.2 Crux (VyOS 1.2.0-rc10) board.Dec 5 2018, 11:58 PM
syncer added a project: VyOS-1.2.0-GA.Jan 25 2019, 2:38 PM