From 9ac1a7801b3caa26517a04e7e00d821725f7428a Mon Sep 17 00:00:00 2001 From: Vadim Kurland Date: Wed, 3 Jun 2009 18:24:14 +0000 Subject: [PATCH 1/3] 2009-06-03 vadim * ObjectManipulator.cpp (ObjectManipulator::findWhereUsedRecursively): fixed bug #2800625 "recursive groups cause infinite loop and crash in compiler". When a group included itself, compiler used to go into infinite loop and crash. The fix in this function also takes care of the situation when group A referenced group B, which in turn referenced group A again. --- build_num | 2 +- doc/ChangeLog | 9 +++++++++ src/gui/ObjectManipulator.cpp | 17 +++++++++++------ 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/build_num b/build_num index a4075c4a0..9341fab89 100644 --- a/build_num +++ b/build_num @@ -1 +1 @@ -#define BUILD_NUM 991 +#define BUILD_NUM 1030 diff --git a/doc/ChangeLog b/doc/ChangeLog index 9d9e2bf06..62eb849db 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,3 +1,12 @@ +2009-06-03 vadim + + * ObjectManipulator.cpp (ObjectManipulator::findWhereUsedRecursively): + fixed bug #2800625 "recursive groups cause infinite loop and crash + in compiler". When a group included itself, compiler used to go + into infinite loop and crash. The fix in this function also takes + care of the situation when group A referenced group B, which in + turn referenced group A again. + 2009-06-01 vadim * newHostDialog.cpp (newHostDialog::selectedInterface): fixed the diff --git a/src/gui/ObjectManipulator.cpp b/src/gui/ObjectManipulator.cpp index bbfcda8dc..6dac38454 100644 --- a/src/gui/ObjectManipulator.cpp +++ b/src/gui/ObjectManipulator.cpp @@ -3211,11 +3211,19 @@ void ObjectManipulator::findWhereUsedRecursively(FWObject *obj, obj->getName().c_str(), obj->getTypeName().c_str()); set resset_tmp; - set resset_tmp2; +/* + * findWhereObjectIsUsed() finds references to object 'obj' in a subtree + * rooted at object 'top'. + */ m_project->db()->findWhereObjectIsUsed(obj, top, resset_tmp); + set::iterator i = resset.begin(); + for ( ; i!=resset.end(); ++i) + if (resset_tmp.count(*i)) resset_tmp.erase(*i); - set::iterator i = resset_tmp.begin(); + resset.insert(resset_tmp.begin(), resset_tmp.end()); + + i = resset_tmp.begin(); for ( ; i!=resset_tmp.end(); ++i) { FWObject *parent_obj = *i; @@ -3226,11 +3234,8 @@ void ObjectManipulator::findWhereUsedRecursively(FWObject *obj, // add new results to a separate set to avoid modifying the resset_tmp // in the middle of iteration if (Group::cast(parent_obj) && !RuleElement::cast(parent_obj)) - findWhereUsedRecursively(parent_obj, top, resset_tmp2); + findWhereUsedRecursively(parent_obj, top, resset); } - - resset.insert(resset_tmp.begin(), resset_tmp.end()); - resset.insert(resset_tmp2.begin(), resset_tmp2.end()); } list ObjectManipulator::findFirewallsForObject(FWObject *o) From 94ac7dd95594afdccd6c7ec6d5191d1fc2c424b4 Mon Sep 17 00:00:00 2001 From: Vadim Kurland Date: Thu, 4 Jun 2009 20:35:53 +0000 Subject: [PATCH 2/3] 2009-06-04 vadim * IPTImporter.cpp (IPTImporter::pushPolicyRule): fixed bug #2801362 "Iptables policy import does not handle rules with ESTABLISED". Policy importer for iptables should properly handle rules that use combination of a "-p protocol" and match state "RELATED,ESTABLISHED". Example: -A INBOUND -p tcp -m state --state RELATED,ESTABLISHED -j ACCEPT This rule should translate into fwbuilder rule using CustomService object with code "-m state --state RELATED,ESTABLISHED" and protocol spec "tcp". --- build_num | 2 +- doc/ChangeLog | 12 ++++++++++++ src/gui/IPTImporter.cpp | 40 +++++++++++++++++++++++++++++----------- src/gui/Importer.cpp | 33 +++++++++++++++++++++++++++++++++ src/gui/Importer.h | 8 +++++++- 5 files changed, 82 insertions(+), 13 deletions(-) diff --git a/build_num b/build_num index 9341fab89..0a23b96da 100644 --- a/build_num +++ b/build_num @@ -1 +1 @@ -#define BUILD_NUM 1030 +#define BUILD_NUM 1035 diff --git a/doc/ChangeLog b/doc/ChangeLog index 62eb849db..e5e2ef2f5 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,3 +1,15 @@ +2009-06-04 vadim + + * IPTImporter.cpp (IPTImporter::pushPolicyRule): fixed bug + #2801362 "Iptables policy import does not handle rules with + ESTABLISED". Policy importer for iptables should properly + handle rules that use combination of a "-p protocol" and + match state "RELATED,ESTABLISHED". Example: + -A INBOUND -p tcp -m state --state RELATED,ESTABLISHED -j ACCEPT + This rule should translate into fwbuilder rule using CustomService + object with code "-m state --state RELATED,ESTABLISHED" + and protocol spec "tcp". + 2009-06-03 vadim * ObjectManipulator.cpp (ObjectManipulator::findWhereUsedRecursively): diff --git a/src/gui/IPTImporter.cpp b/src/gui/IPTImporter.cpp index 146b52748..65dcdd352 100644 --- a/src/gui/IPTImporter.cpp +++ b/src/gui/IPTImporter.cpp @@ -56,6 +56,8 @@ #include "fwbuilder/Policy.h" #include "fwbuilder/NAT.h" #include "fwbuilder/RuleElement.h" +#include "fwbuilder/FWServiceReference.h" +#include "fwbuilder/CustomService.h" using namespace libfwbuilder; @@ -565,18 +567,34 @@ void IPTImporter::pushPolicyRule() rule->getSrv()->setNeg(srv_neg); rule->getItf()->setNeg(intf_neg); - if (rule->getSrc()->isAny() && - rule->getDst()->isAny() && - rule->getSrv()->isAny() && - current_state == "RELATED,ESTABLISHED") + if (current_state == "RELATED,ESTABLISHED") { - fwopt->setBool("accept_established", true); - skip_rule = true; - *Importer::logger - << "Using automatic rule controlled by option " - << "'Accept established,related states' to match " - << "states RELATED,ESTABLISHED" - << "\n"; + if (rule->getSrc()->isAny() && + rule->getDst()->isAny() && + rule->getSrv()->isAny()) + { + fwopt->setBool("accept_established", true); + skip_rule = true; + *Importer::logger + << "Using automatic rule controlled by option " + << "'Accept established,related states' to match " + << "states RELATED,ESTABLISHED" + << "\n"; + } else + { + RuleElementSrv *srv = rule->getSrv(); + std::string protocol = ""; + if (!rule->getSrv()->isAny()) + { + Service *srv_obj = Service::cast(FWServiceReference::getObject( + srv->front())); + protocol = srv_obj->getProtocolName(); + } + FWObject *established = getCustomService( + "iptables", "-m state --state RELATED,ESTABLISHED", protocol); + srv->clearChildren(); + srv->addRef(established); + } } if (rule->getSrc()->isAny() && diff --git a/src/gui/Importer.cpp b/src/gui/Importer.cpp index 8c82bf4bc..cd89d63b6 100644 --- a/src/gui/Importer.cpp +++ b/src/gui/Importer.cpp @@ -46,6 +46,7 @@ #include "fwbuilder/ICMPService.h" #include "fwbuilder/TCPService.h" #include "fwbuilder/UDPService.h" +#include "fwbuilder/CustomService.h" #include "fwbuilder/TagService.h" #include "fwbuilder/Resources.h" #include "fwbuilder/Policy.h" @@ -125,6 +126,7 @@ Importer::Importer(FWObject *_lib, current_interface = NULL; current_ruleset = NULL; current_rule = NULL; + custom_service_code_tracker = 0; tcp_flag_names[libfwbuilder::TCPService::URG]="u"; tcp_flag_names[libfwbuilder::TCPService::ACK]="a"; @@ -494,6 +496,37 @@ Firewall* Importer::finalize() return fw; } +FWObject* Importer::getCustomService(const std::string &platform, + const std::string &code, + const std::string &protocol) +{ + // this assumes protocol is represented by a number + if (custom_service_codes.count(platform + code)==0) + { + custom_service_codes[platform + code] = custom_service_code_tracker; + custom_service_code_tracker++; + } + + std::ostringstream nstr, cstr, sstr; + nstr << "cust-" << custom_service_codes[platform + code] << "-" << protocol; + sstr << "cust-" << custom_service_codes[platform + code] << "-" << protocol; + cstr << "Imported from " << getFirewallObject()->getName() << "\n" + << "code: " << code << "\n" + << "protocol: " << protocol; + + if (all_objects.count(sstr.str())!=0) return all_objects[sstr.str()]; + + CustomService *s = CustomService::cast( + createObject(CustomService::TYPENAME, nstr.str())); + if (!protocol.empty()) s->setProtocol(protocol); + s->setCodeForPlatform(platform, code); + s->setComment(cstr.str()); + all_objects[sstr.str()] = s; + + *logger << "Custom Service object: " << nstr.str() << "\n"; + return s; +} + FWObject* Importer::getIPService(int proto) { // this assumes protocol is represented by a number diff --git a/src/gui/Importer.h b/src/gui/Importer.h index 40b477e40..792da7b06 100644 --- a/src/gui/Importer.h +++ b/src/gui/Importer.h @@ -105,7 +105,10 @@ protected: // map : object signature : object // use this to quickly find objects to avoid creating duplicates std::map all_objects; - + + int custom_service_code_tracker; + std::map custom_service_codes; + UnidirectionalRuleSet* current_ruleset; libfwbuilder::Rule* current_rule; @@ -140,6 +143,9 @@ protected: // exists, it is created UnidirectionalRuleSet* getUnidirRuleSet(const std::string &rsname); + virtual libfwbuilder::FWObject* getCustomService(const std::string &platform, + const std::string &code, + const std::string &protocol); virtual libfwbuilder::FWObject* getIPService(int proto); virtual libfwbuilder::FWObject* getICMPService(int type, int code); From 08152758730be83045d712322042ecce5890e2c6 Mon Sep 17 00:00:00 2001 From: Vadim Kurland Date: Fri, 5 Jun 2009 16:58:28 +0000 Subject: [PATCH 3/3] 2009-06-05 vadim * PolicyCompiler_PrintRule.cpp (PrintRule::_printIP): fixed bug #2801548 "fwb_ipt should issue error for ipsrv with options for ipv6". Since IP options lsrr, ssrr, rr do not exist in ipv6, compiler should refuse to compile rules that request matching these options. * PolicyCompiler_iosacl_writers.cpp (PrintRule::_printIPServiceOptions): fixed bug #2801547 "fwb_iosacl should issue an error for ipservice with options". IOS access lists can not match source routing options set in IPService object, compiler should issue an error and abort processing when an object like this is encountered in a rule. * IPServiceDialog.cpp (IPServiceDialog::loadFWObject): fixed bug #2801545 "IP Service object: lsrr, ssrr, rr options not saved". * PolicyCompiler_pf_writers.cpp (PrintRule::_printDstService): fixed bug #2801544 "missing space after tos option in pf config" --- build_num | 2 +- doc/ChangeLog | 21 ++++++ src/gui/IPServiceDialog.cpp | 6 +- src/iosacl/PolicyCompiler_iosacl.h | 3 +- src/iosacl/PolicyCompiler_iosacl_writers.cpp | 23 +++--- src/ipt/PolicyCompiler_PrintRule.cpp | 76 ++++++++++---------- src/ipt/PolicyCompiler_ipt.h | 3 +- src/pflib/PolicyCompiler_pf_writers.cpp | 2 +- test/pf/objects-for-regression-tests.fwb | 11 +-- 9 files changed, 78 insertions(+), 69 deletions(-) diff --git a/build_num b/build_num index 0a23b96da..ed78587e6 100644 --- a/build_num +++ b/build_num @@ -1 +1 @@ -#define BUILD_NUM 1035 +#define BUILD_NUM 1036 diff --git a/doc/ChangeLog b/doc/ChangeLog index e5e2ef2f5..2487fd7bc 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,3 +1,24 @@ +2009-06-05 vadim + + * PolicyCompiler_PrintRule.cpp (PrintRule::_printIP): fixed bug + #2801548 "fwb_ipt should issue error for ipsrv with options for + ipv6". Since IP options lsrr, ssrr, rr do not exist in ipv6, + compiler should refuse to compile rules that request matching + these options. + + * PolicyCompiler_iosacl_writers.cpp (PrintRule::_printIPServiceOptions): + fixed bug #2801547 "fwb_iosacl should issue an error for ipservice + with options". IOS access lists can not match source routing + options set in IPService object, compiler should issue an error + and abort processing when an object like this is encountered in a + rule. + + * IPServiceDialog.cpp (IPServiceDialog::loadFWObject): fixed bug + #2801545 "IP Service object: lsrr, ssrr, rr options not saved". + + * PolicyCompiler_pf_writers.cpp (PrintRule::_printDstService): + fixed bug #2801544 "missing space after tos option in pf config" + 2009-06-04 vadim * IPTImporter.cpp (IPTImporter::pushPolicyRule): fixed bug diff --git a/src/gui/IPServiceDialog.cpp b/src/gui/IPServiceDialog.cpp index a94a9c1df..89cdf7ba7 100644 --- a/src/gui/IPServiceDialog.cpp +++ b/src/gui/IPServiceDialog.cpp @@ -91,9 +91,9 @@ void IPServiceDialog::loadFWObject(FWObject *o) m_dialog->obj_name->setText( QString::fromUtf8(s->getName().c_str()) ); m_dialog->protocolNum->setValue( s->getProtocolNumber() ); - m_dialog->lsrr->setChecked( s->getBool("m_dialog->lsrr") ); - m_dialog->ssrr->setChecked( s->getBool("m_dialog->ssrr") ); - m_dialog->rr->setChecked( s->getBool("m_dialog->rr") ); + m_dialog->lsrr->setChecked( s->getBool("lsrr") ); + m_dialog->ssrr->setChecked( s->getBool("ssrr") ); + m_dialog->rr->setChecked( s->getBool("rr") ); m_dialog->timestamp->setChecked( s->getBool("ts") ); m_dialog->all_fragments->setChecked( s->getBool("fragm") ); m_dialog->short_fragments->setChecked( s->getBool("short_fragm") ); diff --git a/src/iosacl/PolicyCompiler_iosacl.h b/src/iosacl/PolicyCompiler_iosacl.h index 0941782e2..95ab0781e 100644 --- a/src/iosacl/PolicyCompiler_iosacl.h +++ b/src/iosacl/PolicyCompiler_iosacl.h @@ -195,8 +195,7 @@ namespace fwcompiler { std::string _printAction(libfwbuilder::PolicyRule *r); std::string _printACL(libfwbuilder::PolicyRule *r); std::string _printLog(libfwbuilder::PolicyRule *r); - std::string _printFragm(libfwbuilder::Service *srv); - std::string _printTOS(libfwbuilder::Service *srv); + std::string _printIPServiceOptions(libfwbuilder::PolicyRule *r); std::string _printRule(libfwbuilder::PolicyRule *rule); diff --git a/src/iosacl/PolicyCompiler_iosacl_writers.cpp b/src/iosacl/PolicyCompiler_iosacl_writers.cpp index 76d751861..cd81b29d0 100644 --- a/src/iosacl/PolicyCompiler_iosacl_writers.cpp +++ b/src/iosacl/PolicyCompiler_iosacl_writers.cpp @@ -275,8 +275,7 @@ string PolicyCompiler_iosacl::PrintRule::_printRule(PolicyRule *rule) aclstr << _printDstService( compiler->getFirstSrv(rule) ); aclstr << _printLog( rule ); // "fragments" should be the last option in the access-list command - aclstr << _printFragm( compiler->getFirstSrv(rule) ); - aclstr << _printTOS( compiler->getFirstSrv(rule) ); + aclstr << _printIPServiceOptions(rule); // aclstr << endl; @@ -345,20 +344,20 @@ string PolicyCompiler_iosacl::PrintRule::_printSrcService(Service *srv) return str.str(); } -string PolicyCompiler_iosacl::PrintRule::_printFragm(Service *srv) -{ - if (IPService::isA(srv) && ( - srv->getBool("fragm") || srv->getBool("short_fragm"))) - return "fragments "; - - return ""; -} - -string PolicyCompiler_iosacl::PrintRule::_printTOS(Service *srv) +string PolicyCompiler_iosacl::PrintRule::_printIPServiceOptions(PolicyRule *r) { + Service *srv = compiler->getFirstSrv(r); const IPService *ip; if ((ip=IPService::constcast(srv))!=NULL) { + if (ip->getBool("lsrr") || ip->getBool("ssrr") || ip->getBool("rr")) + compiler->abort( + string("Source routing options match is not supported. Rule ") + + r->getLabel()); + + if (srv->getBool("fragm") || srv->getBool("short_fragm")) + return "fragments "; + string tos = ip->getTOSCode(); string dscp = ip->getDSCPCode(); if (!dscp.empty()) return string("dscp ") + dscp; diff --git a/src/ipt/PolicyCompiler_PrintRule.cpp b/src/ipt/PolicyCompiler_PrintRule.cpp index 83cef1d1d..7050f942b 100644 --- a/src/ipt/PolicyCompiler_PrintRule.cpp +++ b/src/ipt/PolicyCompiler_PrintRule.cpp @@ -805,49 +805,47 @@ string PolicyCompiler_ipt::PrintRule::_printICMP(ICMPService *srv) return str.str(); } -string PolicyCompiler_ipt::PrintRule::_printIP(IPService *srv) +string PolicyCompiler_ipt::PrintRule::_printIP(IPService *srv, PolicyRule *rule) { PolicyCompiler_ipt *ipt_comp=dynamic_cast(compiler); std::ostringstream str; - IPService *ip; - if ((ip=IPService::cast(srv))!=NULL) + if (srv->getBool("fragm") || srv->getBool("short_fragm")) { - if (srv->getBool("fragm") || srv->getBool("short_fragm")) - { - if (ipt_comp->ipv6) str << " -m frag --fragmore"; - else str << " -f "; - } - - string tos = ip->getTOSCode(); - string dscp = ip->getDSCPCode(); - if (!tos.empty()) - str << " -m tos --tos " << tos; - else - if (!dscp.empty()) - { - if (dscp.find("BE")==0 || - dscp.find("EF")==0 || - dscp.find("AF")==0 || - dscp.find("CS")==0) - str << " -m dscp --dscp-class " << dscp; - else - str << " -m dscp --dscp " << dscp; - } - - - if (!ipt_comp->ipv6) - { - if (srv->getBool("lsrr") || - srv->getBool("ssrr") || - srv->getBool("rr") || - srv->getBool("ts") ) str << " -m ipv4options "; - - if (srv->getBool("lsrr")) str << " --lsrr"; - if (srv->getBool("ssrr")) str << " --ssrr"; - if (srv->getBool("rr")) str << " --rr"; - if (srv->getBool("ts")) str << " --ts"; - } + if (ipt_comp->ipv6) str << " -m frag --fragmore"; + else str << " -f "; } + + string tos = srv->getTOSCode(); + string dscp = srv->getDSCPCode(); + if (!tos.empty()) + str << " -m tos --tos " << tos; + else + if (!dscp.empty()) + { + if (dscp.find("BE")==0 || + dscp.find("EF")==0 || + dscp.find("AF")==0 || + dscp.find("CS")==0) + str << " -m dscp --dscp-class " << dscp; + else + str << " -m dscp --dscp " << dscp; + } + + if (!ipt_comp->ipv6) + { + if (srv->getBool("lsrr") || + srv->getBool("ssrr") || + srv->getBool("rr") || + srv->getBool("ts") ) str << " -m ipv4options "; + + if (srv->getBool("lsrr")) str << " --lsrr"; + if (srv->getBool("ssrr")) str << " --ssrr"; + if (srv->getBool("rr")) str << " --rr"; + if (srv->getBool("ts")) str << " --ts"; + } else + compiler->abort( + string("IP options match is not supported for IPv6. Rule ") + + rule->getLabel()); return str.str(); } @@ -1018,7 +1016,7 @@ string PolicyCompiler_ipt::PrintRule::_printDstService(RuleElementSrv *rel) } if (IPService::isA(srv)) { - string str=_printIP( IPService::cast(srv) ); + string str = _printIP(IPService::cast(srv), PolicyRule::cast(rel->getParent())); if (! str.empty() ) { ostr << _printSingleObjectNegation(rel) diff --git a/src/ipt/PolicyCompiler_ipt.h b/src/ipt/PolicyCompiler_ipt.h index ac499cf9c..dfbb94fd1 100644 --- a/src/ipt/PolicyCompiler_ipt.h +++ b/src/ipt/PolicyCompiler_ipt.h @@ -890,7 +890,8 @@ namespace fwcompiler { virtual std::string _printSrcPorts(libfwbuilder::Service *srv); virtual std::string _printDstPorts(libfwbuilder::Service *srv); virtual std::string _printICMP(libfwbuilder::ICMPService *srv); - virtual std::string _printIP(libfwbuilder::IPService *srv); + virtual std::string _printIP(libfwbuilder::IPService *srv, + libfwbuilder::PolicyRule *rule); virtual std::string _printTCPFlags(libfwbuilder::TCPService *srv); virtual std::string _printSrcAddr(libfwbuilder::RuleElement *rel, libfwbuilder::Address *o); diff --git a/src/pflib/PolicyCompiler_pf_writers.cpp b/src/pflib/PolicyCompiler_pf_writers.cpp index f78592932..f9a2575d6 100644 --- a/src/pflib/PolicyCompiler_pf_writers.cpp +++ b/src/pflib/PolicyCompiler_pf_writers.cpp @@ -580,7 +580,7 @@ void PolicyCompiler_pf::PrintRule::_printDstService(RuleElementSrv *rel) const IPService *ip = IPService::constcast(srv); string tos = ip->getTOSCode(); string dscp = ip->getDSCPCode(); - if (!tos.empty()) compiler->output << " tos " << tos; + if (!tos.empty()) compiler->output << " tos " << tos << " "; if (!dscp.empty()) compiler->abort("PF does not support DSCP matching"); } diff --git a/test/pf/objects-for-regression-tests.fwb b/test/pf/objects-for-regression-tests.fwb index b81276db9..48b9995ae 100644 --- a/test/pf/objects-for-regression-tests.fwb +++ b/test/pf/objects-for-regression-tests.fwb @@ -406,15 +406,6 @@ - - - - - - - - - @@ -5302,7 +5293,7 @@ - +