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.