diff --git a/doc/ChangeLog b/doc/ChangeLog index 6c0c4d20f..bc9c2f7a9 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,4 +1,7 @@ -2011-03-23 Vadim Kurland +2011-03-23 vadim + + * addressObjectMaker.cpp (createObject): see #1548 Improved + algorithm used to deduplicate Network objects on import. * FWWindow.cpp (prepareToolsMenu): fixed SF bug 3238026: build failure on systems without net-snmp development libraries. diff --git a/src/import/IPTImporter.cpp b/src/import/IPTImporter.cpp index 7766f8df3..6b7fe7206 100644 --- a/src/import/IPTImporter.cpp +++ b/src/import/IPTImporter.cpp @@ -288,12 +288,12 @@ FWObject* IPTImporter::createTCPUDPService(const std::string &proto) } } -FWObject* IPTImporter::createTCPService(const QString &name) +FWObject* IPTImporter::createTCPService(const QString &) { return createTCPUDPService("tcp"); } -FWObject* IPTImporter::createUDPService(const QString &name) +FWObject* IPTImporter::createUDPService(const QString &) { return createTCPUDPService("udp"); } @@ -305,8 +305,8 @@ FWObject* IPTImporter::makeSrcObj() { ObjectSignature sig; sig.type_name = AddressRange::TYPENAME; - sig.address_range_start = iprange_src_from.c_str(); - sig.address_range_end = iprange_src_to.c_str(); + sig.setAddressRangeStart(iprange_src_from.c_str()); + sig.setAddressRangeEnd(iprange_src_to.c_str()); return commitObject(address_maker->createObject(sig)); } else @@ -319,8 +319,8 @@ FWObject* IPTImporter::makeDstObj() { ObjectSignature sig; sig.type_name = AddressRange::TYPENAME; - sig.address_range_start = iprange_dst_from.c_str(); - sig.address_range_end = iprange_dst_to.c_str(); + sig.setAddressRangeStart(iprange_dst_from.c_str()); + sig.setAddressRangeEnd(iprange_dst_to.c_str()); return commitObject(address_maker->createObject(sig)); } else @@ -1207,15 +1207,15 @@ void IPTImporter::pushNATRule() { ObjectSignature sig; sig.type_name = AddressRange::TYPENAME; - sig.address_range_start = nat_addr1.c_str(); - sig.address_range_end = nat_addr2.c_str(); + sig.setAddressRangeStart(nat_addr1.c_str()); + sig.setAddressRangeEnd(nat_addr2.c_str()); tsrc = commitObject(address_maker->createObject(sig)); } else { ObjectSignature sig; sig.type_name = Address::TYPENAME; - sig.address = nat_addr1.c_str(); - sig.netmask = nat_nm.c_str(); + sig.setAddress(nat_addr1.c_str()); + sig.setNetmask(nat_nm.c_str()); tsrc = commitObject(address_maker->createObject(sig)); } @@ -1261,15 +1261,15 @@ void IPTImporter::pushNATRule() { ObjectSignature sig; sig.type_name = AddressRange::TYPENAME; - sig.address_range_start = nat_addr1.c_str(); - sig.address_range_end = nat_addr2.c_str(); + sig.setAddressRangeStart(nat_addr1.c_str()); + sig.setAddressRangeEnd(nat_addr2.c_str()); tdst = commitObject(address_maker->createObject(sig)); } else { ObjectSignature sig; sig.type_name = Address::TYPENAME; - sig.address = nat_addr1.c_str(); - sig.netmask = nat_nm.c_str(); + sig.setAddress(nat_addr1.c_str()); + sig.setNetmask(nat_nm.c_str()); tdst = commitObject(address_maker->createObject(sig)); } @@ -1338,8 +1338,8 @@ void IPTImporter::pushNATRule() ObjectSignature sig; sig.type_name = Address::TYPENAME; - sig.address = nat_addr1.c_str(); - sig.netmask = nat_nm.c_str(); + sig.setAddress(nat_addr1.c_str()); + sig.setNetmask(nat_nm.c_str()); o = commitObject(address_maker->createObject(sig)); tsrc->addRef(o); } @@ -1353,8 +1353,8 @@ void IPTImporter::pushNATRule() ObjectSignature sig; sig.type_name = Address::TYPENAME; - sig.address = nat_addr1.c_str(); - sig.netmask = nat_nm.c_str(); + sig.setAddress(nat_addr1.c_str()); + sig.setNetmask(nat_nm.c_str()); o = commitObject(address_maker->createObject(sig)); tdst->addRef(o); } diff --git a/src/import/Importer.cpp b/src/import/Importer.cpp index 4f5820a77..b03b4cd08 100644 --- a/src/import/Importer.cpp +++ b/src/import/Importer.cpp @@ -544,8 +544,8 @@ FWObject* Importer::makeSrcObj() ObjectSignature sig; sig.type_name = Address::TYPENAME; - sig.address = src_a.c_str(); - sig.netmask = src_nm.c_str(); + sig.setAddress(src_a.c_str()); + sig.setNetmask(src_nm.c_str(), address_maker->getInvertedNetmasks()); return commitObject(address_maker->createObject(sig)); } @@ -560,8 +560,8 @@ FWObject* Importer::makeDstObj() ObjectSignature sig; sig.type_name = Address::TYPENAME; - sig.address = dst_a.c_str(); - sig.netmask = dst_nm.c_str(); + sig.setAddress(dst_a.c_str()); + sig.setNetmask(dst_nm.c_str(), address_maker->getInvertedNetmasks()); return commitObject(address_maker->createObject(sig)); } diff --git a/src/import/PIXImporter.cpp b/src/import/PIXImporter.cpp index 0cccc9e40..f91cf99f9 100644 --- a/src/import/PIXImporter.cpp +++ b/src/import/PIXImporter.cpp @@ -425,8 +425,8 @@ void PIXImporter::commitNamedAddressRangeObject() ObjectSignature sig; sig.object_name = named_object_name; sig.type_name = AddressRange::TYPENAME; - sig.address_range_start = tmp_range_1.c_str(); - sig.address_range_end = tmp_range_2.c_str(); + sig.setAddressRangeStart(tmp_range_1.c_str()); + sig.setAddressRangeEnd(tmp_range_2.c_str()); current_named_object = commitObject(address_maker->createObject(sig)); named_objects_registry[named_object_name] = current_named_object; } @@ -594,8 +594,8 @@ void PIXImporter::addNetworkToObjectGroup() { ObjectSignature sig; sig.type_name = Address::TYPENAME; - sig.address = tmp_a.c_str(); - sig.netmask = tmp_nm.c_str(); + sig.setAddress(tmp_a.c_str()); + sig.setNetmask(tmp_nm.c_str()); current_object_group->addRef( commitObject(address_maker->createObject(sig))); } diff --git a/src/import/addressObjectMaker.cpp b/src/import/addressObjectMaker.cpp index 37ddff934..675173f83 100644 --- a/src/import/addressObjectMaker.cpp +++ b/src/import/addressObjectMaker.cpp @@ -49,82 +49,50 @@ AddressObjectMaker::~AddressObjectMaker() {} FWObject* AddressObjectMaker::createObject(ObjectSignature &sig) { -// FWObject *obj = findMatchingObject(sig); -// if (obj) return obj; - FWObject *obj = NULL; if (sig.type_name == AddressRange::TYPENAME) - obj = createAddressRange(sig.address_range_start, sig.address_range_end); + obj = createAddressRange(sig); else - obj = createAddress(sig.address, sig.netmask); + obj = createAddress(sig); + + // Now I should build new signature because actual object type has + // only been determined in createAddress() if ( ! sig.object_name.isEmpty()) { obj->setName(sig.object_name.toUtf8().constData()); - registerNamedObject(sig, obj); + ObjectSignature new_sig; + obj->dispatch(&new_sig, (void*)(NULL)); + registerNamedObject(new_sig, obj); } else - registerAnonymousObject(sig, obj); + { + ObjectSignature new_sig; + obj->dispatch(&new_sig, (void*)(NULL)); + registerAnonymousObject(new_sig, obj); + } return obj; } - -FWObject* AddressObjectMaker::createAddress(const QString &addr, - const QString &netmask) +FWObject* AddressObjectMaker::createAddress(ObjectSignature &sig) { - QString correct_nm = netmask; - if (inverted_netmasks) - { - InetAddr orig_nm(netmask.toStdString()); - correct_nm = (~orig_nm).toString().c_str(); - } + ObjectSignature signature = sig; - try - { - InetAddr(correct_nm.toStdString()); - } catch (FWException &ex) - { - if (correct_nm.contains('.')) - { - // netmask has '.' in it but conversion failed. - throw ObjectMakerException( - QString("Error converting netmask '%1'").arg(correct_nm)); - } else - { - // no dot in netmask, perhaps it is specified by its length? - // if netmask is specified by length, need to use special - // constructor for class Netmask to convert - bool ok = false; - int nm_len = correct_nm.toInt(&ok); - if (ok) - { - correct_nm = InetAddr(nm_len).toString().c_str(); - } else - { - // could not convert netmask as simple integer - throw ObjectMakerException( - QString("Error converting netmask '%1'").arg(correct_nm)); - } - } - } + InetAddr netmask(signature.netmask.toStdString()); - ObjectSignature sig; - sig.address = addr; - sig.netmask = correct_nm; - - if ( correct_nm == InetAddr::getAllOnes().toString().c_str() ) + if ( netmask == InetAddr::getAllOnes() ) { QString name; try { - sig.type_name = IPv4::TYPENAME; + signature.type_name = IPv4::TYPENAME; - FWObject *obj = findMatchingObject(sig); + FWObject *obj = findMatchingObject(signature); if (obj) return obj; - InetAddr obj_addr(addr.toStdString()); // testing if string converts to an address - name = QString("h-") + addr; + InetAddr obj_addr(sig.address.toStdString()); // testing if string converts to an address + name = QString("h-") + sig.address; Address *a = Address::cast( ObjectMaker::createObject(IPv4::TYPENAME, name.toStdString())); a->setAddress(obj_addr); @@ -137,52 +105,55 @@ FWObject* AddressObjectMaker::createAddress(const QString &addr, // Since parsers do not understand ipv6 yet, assume this // is a host address and create DNSName object - sig.type_name = DNSName::TYPENAME; - FWObject *obj = findMatchingObject(sig); + signature.type_name = DNSName::TYPENAME; + FWObject *obj = findMatchingObject(signature); if (obj) return obj; - name = addr; + name = sig.address; DNSName *da = DNSName::cast( ObjectMaker::createObject(DNSName::TYPENAME, name.toStdString())); - da->setSourceName(addr.toStdString()); + da->setSourceName(sig.address.toStdString()); da->setRunTime(true); return da; } } else { - sig.type_name = Network::TYPENAME; + signature.type_name = Network::TYPENAME; - qDebug() << "Search for " << sig.toString(); - - FWObject *obj = findMatchingObject(sig); + FWObject *obj = findMatchingObject(signature); if (obj) return obj; - QString name = QString("net-") + addr + "/" + correct_nm; + QString name = QString("net-%1/%2") + .arg(signature.address).arg(signature.netmask); Network *net = Network::cast( ObjectMaker::createObject(Network::TYPENAME, name.toStdString())); try { - net->setAddress( InetAddr(addr.toStdString()) ); + net->setAddress( InetAddr(sig.address.toStdString()) ); } catch (FWException &ex) { throw ObjectMakerException( - QString("Error converting address '%1'").arg(addr)); + QString("Error converting address '%1'").arg(sig.address)); } // we have already verified netmask above - net->setNetmask( InetAddr(correct_nm.toStdString()) ); + net->setNetmask(netmask); return net; } return NULL; } -FWObject* AddressObjectMaker::createAddressRange(const QString &addr1, - const QString &addr2) +FWObject* AddressObjectMaker::createAddressRange(ObjectSignature &sig) { + FWObject *obj = findMatchingObject(sig); + if (obj) return obj; + + QString addr1 = sig.address_range_start; + QString addr2 = sig.address_range_end; + QString name = QString("range-%1-%2").arg(addr1).arg(addr2); - QString name = QString("range-") + addr1 + "-" + addr2; AddressRange *ar = AddressRange::cast( ObjectMaker::createObject(AddressRange::TYPENAME, name.toStdString())); diff --git a/src/import/addressObjectMaker.h b/src/import/addressObjectMaker.h index f968ae921..f61505c00 100644 --- a/src/import/addressObjectMaker.h +++ b/src/import/addressObjectMaker.h @@ -43,14 +43,13 @@ public: virtual ~AddressObjectMaker(); void setInvertedNetmasks(bool f) { inverted_netmasks = f; } + bool getInvertedNetmasks() { return inverted_netmasks; } virtual libfwbuilder::FWObject* createObject(ObjectSignature &sig); protected: - virtual libfwbuilder::FWObject* createAddress(const QString &a, - const QString &nm); - virtual libfwbuilder::FWObject* createAddressRange(const QString &a1, - const QString &a2); + virtual libfwbuilder::FWObject* createAddress(ObjectSignature &sig); + virtual libfwbuilder::FWObject* createAddressRange(ObjectSignature &sig); }; diff --git a/src/import/objectMaker.cpp b/src/import/objectMaker.cpp index e207fe21a..410a2ddee 100644 --- a/src/import/objectMaker.cpp +++ b/src/import/objectMaker.cpp @@ -242,6 +242,59 @@ ObjectSignature::ObjectSignature(const ObjectSignature &other) } } +void ObjectSignature::setAddress(const QString &s) +{ + address = s; +} + +void ObjectSignature::setAddressRangeStart(const QString &s) +{ + address_range_start = s; +} + +void ObjectSignature::setAddressRangeEnd(const QString &s) +{ + address_range_end = s; +} + +void ObjectSignature::setNetmask(const QString &netm, bool inverted_netmask) +{ + InetAddr inetaddr_nm; + + try + { + inetaddr_nm = InetAddr(netm.toStdString()); + if (inverted_netmask) inetaddr_nm = ~inetaddr_nm; + + } catch (FWException &ex) + { + if (netm.contains('.')) + { + // netmask has '.' in it but conversion failed. + throw ObjectMakerException( + QString("Error converting netmask '%1'").arg(netm)); + } else + { + // no dot in netmask, perhaps it is specified by its length? + // If netmask is specified by length, need to use special + // constructor for class Netmask to convert + bool ok = false; + int nm_len = netm.toInt(&ok); + if (ok) + { + inetaddr_nm = InetAddr(nm_len); + } else + { + // could not convert netmask as simple integer + throw ObjectMakerException( + QString("Error converting netmask '%1'").arg(netm)); + } + } + } + + netmask = inetaddr_nm.toString().c_str(); +} + void ObjectSignature::setProtocol(const QString &s) { // this assumes protocol is represented by a number @@ -838,8 +891,6 @@ void ObjectMaker::prepareForDeduplication(FWObject *root) root->dispatch(&sig, (void*)(NULL)); - qDebug() << "Registering " << sig.toString(); - registerNamedObject(sig, root); registerAnonymousObject(sig, root); // this erases sig.object_name } diff --git a/src/import/objectMaker.h b/src/import/objectMaker.h index df68070b4..7c8b8810c 100644 --- a/src/import/objectMaker.h +++ b/src/import/objectMaker.h @@ -130,6 +130,10 @@ public: // convenience methods that populate various attributes from // strings taken from imported configs + void setAddress(const QString &s); + void setNetmask(const QString &s, bool inverted_netmask=false); + void setAddressRangeStart(const QString &s); + void setAddressRangeEnd(const QString &s); void setProtocol(const QString &s); void setIcmpFromName(const QString &s); void setIcmpType(const QString &s); diff --git a/src/unit_tests/ImporterTest/test_data/ios.fwb b/src/unit_tests/ImporterTest/test_data/ios.fwb index 5faddbab5..1c1065fb9 100644 --- a/src/unit_tests/ImporterTest/test_data/ios.fwb +++ b/src/unit_tests/ImporterTest/test_data/ios.fwb @@ -1,6 +1,6 @@ - + @@ -476,8 +476,8 @@ - - + + diff --git a/src/unit_tests/ImporterTest/test_data/ios.result b/src/unit_tests/ImporterTest/test_data/ios.result index bcfacce40..44beed24a 100644 --- a/src/unit_tests/ImporterTest/test_data/ios.result +++ b/src/unit_tests/ImporterTest/test_data/ios.result @@ -2,22 +2,22 @@ Host name: "c3620" New interface: FastEthernet0/0 Interface address: 192.168.100.100/255.255.255.0 Interface address: 10.3.14.201/255.255.255.0 -Interface ruleset fe0_0_acl_in direction 'in' (set to 'in') -Interface ruleset fe0_0_acl_out direction 'out' (set to 'out') +Interface FastEthernet0/0 ruleset fe0_0_acl_in direction 'in' +Interface FastEthernet0/0 ruleset fe0_0_acl_out direction 'out' New interface: Ethernet1/0 Interface comment: Test [ test ] { test } ( and one more test) / weird:characters#$%^&*/ Interface address: 192.168.171.2/255.255.255.0 -Interface ruleset e1_0_acl_in direction 'in' (set to 'in') -Interface ruleset e1_0_acl_out direction 'out' (set to 'out') +Interface Ethernet1/0 ruleset e1_0_acl_in direction 'in' +Interface Ethernet1/0 ruleset e1_0_acl_out direction 'out' New interface: Serial1/0 New interface: Ethernet1/1 Interface address: 10.10.10.10/255.255.255.0 -Interface ruleset acl_133 direction 'in' (set to 'in') -Interface ruleset acl_133 direction 'out' (set to 'both') +Interface Ethernet1/1 ruleset acl_133 direction 'in' +Interface Ethernet1/1 ruleset acl_133 direction 'out' New interface: Ethernet1/2 Interface address: 10.10.20.20/255.255.255.0 -Interface ruleset acl_133 direction 'in' (set to 'in') -Interface ruleset acl_133 direction 'out' (set to 'both') +Interface Ethernet1/2 ruleset acl_133 direction 'in' +Interface Ethernet1/2 ruleset acl_133 direction 'out' Ruleset: e1_0_acl_in Ruleset: e1_0_acl_out Ruleset: fe0_0_acl_in diff --git a/src/unit_tests/ImporterTest/test_data/ipt.fwb b/src/unit_tests/ImporterTest/test_data/ipt.fwb index 931f5ef71..0ebe76ef2 100644 --- a/src/unit_tests/ImporterTest/test_data/ipt.fwb +++ b/src/unit_tests/ImporterTest/test_data/ipt.fwb @@ -1,6 +1,6 @@ - + @@ -453,14 +453,14 @@ - - - - - - - - + + + + + + + + diff --git a/src/unit_tests/ImporterTest/test_data/ipt.result b/src/unit_tests/ImporterTest/test_data/ipt.result index a6aeb2b47..8d7a35880 100644 --- a/src/unit_tests/ImporterTest/test_data/ipt.result +++ b/src/unit_tests/ImporterTest/test_data/ipt.result @@ -23,11 +23,7 @@ Created branch Policy_eth1 New interface: eth1 New interface: eth0 Warning: Line 42: Creating branch ruleset 'Policy_eth1' to match inbound and outbound interfaces -i eth0 -o eth1 -Warning: Line 69: Unknown parameter of target REJECT: icmp-foo-prohibited. -Warning: Line 70: Unknown parameter of target REJECT: foo-prohib. Warning: Line 103: Rule matches states 'RELATED,ESTABLISHED'. Consider using automatic rule controlled by the checkbox in the firewall settings dialog. Automatic rule matches in all standard chains which may be different from the original imported configuration. This requires manual checking. -Parser error: Line 150: Port spec foo is unknown -Parser error: Line 150: Port spec foo is unknown Created branch user_chain_42_mod_match Created branch user_chain_43_mod_match Created branch user_chain_44_mod_match