From 0d69945d2feda863b6c0c20478f7e3f94046ea6f Mon Sep 17 00:00:00 2001 From: Vadim Kurland Date: Thu, 7 Jul 2011 18:04:24 -0700 Subject: [PATCH] see #2546 "PF import - negation inside of inline tables is ignored". Since we can not import address lists or tables that contain a mix of negated and non-negated items, importer should display an error when it enounters one of these and mark all rules that use it as "broken" (rule is colored red and error message is added to the comment). --- doc/ChangeLog | 7 + src/import/Importer.cpp | 15 ++ src/import/Importer.h | 11 ++ src/import/PFImporter.cpp | 32 +++-- .../PFImporterTest/test_data/pf-tables.conf | 6 + .../PFImporterTest/test_data/pf-tables.fwb | 132 +++++++++++------- .../PFImporterTest/test_data/pf-tables.output | 7 +- 7 files changed, 148 insertions(+), 62 deletions(-) diff --git a/doc/ChangeLog b/doc/ChangeLog index b5fd9e7a5..22a0711b2 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,5 +1,12 @@ 2011-07-07 Vadim Kurland + * PFImporter.cpp (makeAddressObj): see #2546 "PF import - negation + inside of inline tables is ignored". Since we can not import + address lists or tables that contain a mix of negated and + non-negated items, importer should display an error when it + enounters one of these and mark all rules that use it as "broken" + (rule is colored red and error message is added to the comment). + * PFImporter.cpp (makeAddressObj): see #2556 "PF import: impor of rules referring to undefined macros". If pf.conf file uses an undefined macro (there is $macro somewhere but the macro has never diff --git a/src/import/Importer.cpp b/src/import/Importer.cpp index 263ce2bb4..d55990ca0 100644 --- a/src/import/Importer.cpp +++ b/src/import/Importer.cpp @@ -1016,3 +1016,18 @@ void Importer::rearrangeVlanInterfaces() } +void Importer::registerBrokenObject(FWObject *obj, const QString &err) +{ + broken_objects[obj] = err; +} + +bool Importer::isObjectBroken(FWObject *obj) +{ + return broken_objects.count(obj) != 0; +} + +QString Importer::getBrokenObjectError(FWObject *obj) +{ + return broken_objects[obj]; +} + diff --git a/src/import/Importer.h b/src/import/Importer.h index db3f772fd..9700fb7db 100644 --- a/src/import/Importer.h +++ b/src/import/Importer.h @@ -142,6 +142,13 @@ protected: // use this to quickly find objects to avoid creating duplicates std::map all_objects; + // registry of broken objects. Sometimes we create an AddressTable + // or a group object during import that may have some kind of a problem + // that we leave for the user to fix manually. In order to be able to mark + // all rules that use this object as "broken", we should register these + // broken objects somewhere. + std::map broken_objects; + UnidirectionalRuleSet* current_ruleset; libfwbuilder::Rule* current_rule; @@ -203,6 +210,10 @@ protected: virtual void addOSrv(); virtual void addLogging(); + + void registerBrokenObject(libfwbuilder::FWObject *o, const QString &err); + bool isObjectBroken(libfwbuilder::FWObject*); + QString getBrokenObjectError(libfwbuilder::FWObject*); public: diff --git a/src/import/PFImporter.cpp b/src/import/PFImporter.cpp index 6eb8b0ba4..4cf12d90c 100644 --- a/src/import/PFImporter.cpp +++ b/src/import/PFImporter.cpp @@ -587,7 +587,12 @@ FWObject* PFImporter::makeAddressObj(AddressSpec &as) if (as.at == AddressSpec::TABLE) { - return address_table_registry[as.address.c_str()]; + FWObject *at = address_table_registry[as.address.c_str()]; + if (isObjectBroken(at)) + { + error_tracker->registerError(getBrokenObjectError(at)); + } + return at; } return NULL; @@ -1375,21 +1380,28 @@ void PFImporter::newAddressTableObject(const string &name, .arg(QString::fromUtf8(name.c_str())) .arg(addr_list.join(", "))); - if (has_negations) - { - // can not use error_tracker->registerError() here because - // tables are created before importer encounters any rules and - // so this error can not be associated with a rule. - addMessageToLog( - QObject::tr("Error: import of table definition with negated addresses is not supported.")); - } - ObjectMaker maker(Library::cast(library), error_tracker); FWObject *og = commitObject(maker.createObject(ObjectGroup::TYPENAME, name.c_str())); assert(og!=NULL); address_table_registry[name.c_str()] = og; + if (has_negations) + { + // can not use error_tracker->registerError() here because + // tables are created before importer encounters any rules and + // so this error can not be associated with a rule. + QString err = + QObject::tr("Error: import of table definition with negated " + "addresses is not supported."); + addMessageToLog(err); + + err = + QObject::tr("Address table '%1' has a mix of negated and non-negated " + "addresses in the original file."); + registerBrokenObject(og, err.arg(QString::fromUtf8(name.c_str()))); + } + for (it=addresses.begin(); it!=addresses.end(); ++it) { FWObject *obj = makeAddressObj(*it); diff --git a/src/unit_tests/PFImporterTest/test_data/pf-tables.conf b/src/unit_tests/PFImporterTest/test_data/pf-tables.conf index 85b430f7d..baf82a3da 100644 --- a/src/unit_tests/PFImporterTest/test_data/pf-tables.conf +++ b/src/unit_tests/PFImporterTest/test_data/pf-tables.conf @@ -10,3 +10,9 @@ table { 192.168.1.1, 192.168.1.2, 192.168.2.0/24 } table { pcn0, pcn0:network } table { pcn0:peer, pcn0:0 } table { www.fwbuilder.org, www.netcitadel.com } + +# unsupported: this table has a mix of negated and non-negated addresses +table { 192.168.10.1, !192.168.10.2, 192.168.20.0/24 } + +# the rule should be marked as "broken" +pass in quick on em1 from to any diff --git a/src/unit_tests/PFImporterTest/test_data/pf-tables.fwb b/src/unit_tests/PFImporterTest/test_data/pf-tables.fwb index 34418c699..1d1ffc76a 100644 --- a/src/unit_tests/PFImporterTest/test_data/pf-tables.fwb +++ b/src/unit_tests/PFImporterTest/test_data/pf-tables.fwb @@ -1,6 +1,6 @@ - + @@ -434,75 +434,107 @@ + + - - - + + + - - - - - - + + + + + + - - - - - + + + + + - + - + - - - + + + - - - + + + - + + + + + + - + - - - - - - + + + + + + + - + - - - - - - - - - + + + + + + + + + - - - + + + - + + + + + + + + + + + + + + + + + + + + + + - + - + + + + + - @@ -522,7 +554,7 @@ - - + + diff --git a/src/unit_tests/PFImporterTest/test_data/pf-tables.output b/src/unit_tests/PFImporterTest/test_data/pf-tables.output index d935e7866..5a1242668 100644 --- a/src/unit_tests/PFImporterTest/test_data/pf-tables.output +++ b/src/unit_tests/PFImporterTest/test_data/pf-tables.output @@ -12,5 +12,8 @@ 10: New interface: pcn0 11: Address Table: : pcn0, pcn0 12: Address Table: : www.fwbuilder.org, www.netcitadel.com -Could not find enough information in the data file to create any firewall rules. - +15: Address Table: : 192.168.10.1, !192.168.10.2, 192.168.20.0/24 +15: Error: import of table definition with negated addresses is not supported. +18: New interface: em1 +18: filtering rule: action pass; interfaces: em1 +18: Error: Address table 'dst_addresses_5' has a mix of negated and non-negated addresses in the original file.