Repository: secdev/scapy
Pull request: Backward compatibility: Add Deprecated fields
Rationale: Scapy is pretty bad at keeping things backward compatible. Fields are renamed often for consistency, therefore breaking older stuff. Moreover, because the release schedule is so slow, people aren't warned of changes.

This PR is an attempt to improve this by:
- implementing `alias_fields` that redirects to another field with an optional`DeprecationWarning`. We already have use cases for that: https://github.com/secdev/scapy/issues/1979 https://github.com/secdev/scapy/pull/1855. This would allow to migrate old scapy fields to newer names, without breaking too much stuff too quickly
- fixes https://github.com/secdev/scapy/issues/1979

Not sure how this affects performance. Adding stuff to `get_field` & co never is great

Problem statement:
RadioTap Dummy Channel vs. Frequency confusion
#### Brief description
RadioTap layer seems to confuse what the Frequency is vs. what the Channel is.

#### Environment

-   `scapy===git-archive.deve44bd373f`
-   `3.6`
-   `Ubuntu 18.04`

#### How to reproduce
- Simply capture 802.11 traffic and check out the "Channel" field.

#### Actual result
- Channel values such as `2452`

#### Expected result
- The actual channel, such as channel 9 in the above example.
- A separate field called "Frequency" that would contain `2452` from above.

#### Related resources

Hints from issue discussion:
Hi & thanks for the report
Could you please share a pcap or a packet as bytes/hex so that we can reproduce ?

We recently did several overhauls to RadioTap, it could have broken something.

Thanks !
Sure. Attached
[example.zip](https://github.com/secdev/scapy/files/3050616/example.zip)

That's not correct. In reality, there is only 1 channel field (and a tags one), which stores the frequency of the channel. Scapy does not interpret, thus unlike wireshark, it does not create "fake" fields to add extra informations.

 When you see "channel 9", what happens in reality is that Wireshark matches the frequency using the following table:
![DAB70E02-FD20-4A52-893D-68AF2ACACB33](https://user-images.githubusercontent.com/10530980/55746396-dc780680-5a39-11e9-8c32-20d4322e94e7.jpeg)

This is technically not a bug. We could however potentially edit the Channel field to display the channel number between parenthesis (using `LEShortEnumField`), it that's any clearer. Feel free to submit a PR
Yep, definitely aware of the frequency vs channel (thus the title and description of this ticket). However, Scapy is indeed either mis-representing the data or improperly parsing it.

RadioTap Defines the channel field here: https://www.radiotap.org/fields/Channel.html

Note, it is explicit that the channel field is comprised of __two__ subfields:
- frequency <-- this is what scapy defines as "Channel"
- flags

Correspondingly, scapy exposes a "Channel" field in the radiotap layer, which appears to actually be the frequency subfield. It further exposes "ChannelFlags" and "ChannelFrequency" which appear to not actually get populated. In this way, the "Channel" field in scapy is incorrect, since it does not include flags as defined by radiotap.

To truly not interpret the underlying information, the Channel field in radiotap layer would be 4 bytes and include the flags.

Note that radiotap does a better job at avoiding some of the confusion since it explicitly calls this frequency, since 802.11 defines channel as frequency bands.

I see your point.
Currently:
- we use `ChannelFrequency` for `XChannel.freq` https://www.radiotap.org/fields/XChannel
- we use `Channel` for the real channel frequency, because that's what wireshark used to do before switching to `channel.freq`
![6B9924D9-D802-4E22-A4B7-67108F2B449B](https://user-images.githubusercontent.com/10530980/55751887-0257d800-5a47-11e9-9cbd-2b36b639eafd.jpeg)

We could change the names, but that would break already existing code :/ the issue here is about the naming convention. The parsing is correct
Hmm.. Would it break functionality to also add the frequency to
ChannelFrequency variable that doesn't seem to get populated?

On Mon, Apr 8, 2019 at 3:41 PM Gabriel <notifications@github.com> wrote:

> I see your point.
> Currently:
>
>    - we use ChannelFrequency for XChannel.freq
>    https://www.radiotap.org/fields/XChannel
>    - we use Channel for the real channel frequency, because that's what
>    wireshark used to do before switching to channel.freq
>    [image: 6B9924D9-D802-4E22-A4B7-67108F2B449B]
>    <https://user-images.githubusercontent.com/10530980/55751887-0257d800-5a47-11e9-9cbd-2b36b639eafd.jpeg>
>
> We could change the names, but that would break already existing code :/
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/secdev/scapy/issues/1979#issuecomment-480977977>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AHR34Pq6BjE2OAJfHWXLNAO_xDx6RrRcks5ve5t1gaJpZM4cgOSb>
> .
>

Indeed. Just attempting to identify a way to make the attributes in your
tool less misleading.

Imho the proper answer from both a pythonic and following standards
approach would be to change channel into a class object that exposes the
two sub properties. Thus channel.frequency could be used. And for the
concern about breaking things already written for the current way, simply
override pythons duck typing for the class.

On Fri, Apr 12, 2019, 9:35 AM Gabriel <notifications@github.com> wrote:

> It's normal that it doesn't get populated: ChannelFrequency is used for
> XChannel ! I literally said that above
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/secdev/scapy/issues/1979#issuecomment-482576666>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AHR34PPGpOC-sPFjinAezL8N4LM_l7viks5vgIuPgaJpZM4cgOSb>
> .
>

https://github.com/secdev/scapy/pull/1983/commits/2a1cf29bdf1ab03e9357d81a41b9f7436bebee8b (https://github.com/secdev/scapy/pull/1983) should resolve the issue :smile: Feel free to have a look

Commit message: Add version tag

Patch to review:
diff --git a/scapy/layers/dot11.py b/scapy/layers/dot11.py
index 397d4ff8d52..ab18230a95c 100644
--- a/scapy/layers/dot11.py
+++ b/scapy/layers/dot11.py
@@ -247,6 +247,11 @@ def guess_payload_class(self, pay):
 
 class RadioTap(Packet):
     name = "RadioTap dummy"
+    deprecated_fields = {
+        "Channel": ("ChannelFrequency", "2.4.3"),
+        "ChannelFlags2": ("ChannelPlusFlags", "2.4.3"),
+        "ChannelNumber": ("ChannelPlusNumber", "2.4.3"),
+    }
     fields_desc = [
         ByteField('version', 0),
         ByteField('pad', 0),
@@ -278,7 +283,7 @@ class RadioTap(Packet):
         # Channel
         ConditionalField(
             _RadiotapReversePadField(
-                LEShortField("Channel", 0)
+                LEShortField("ChannelFrequency", 0)
              ),
             lambda pkt: pkt.present and pkt.present.Channel),
         ConditionalField(
@@ -323,14 +328,14 @@ class RadioTap(Packet):
         # ChannelPlus
         ConditionalField(
             _RadiotapReversePadField(
-                FlagsField("ChannelFlags2", None, -32, _rt_channelflags2)
+                FlagsField("ChannelPlusFlags", None, -32, _rt_channelflags2)
             ),
             lambda pkt: pkt.present and pkt.present.ChannelPlus),
         ConditionalField(
-            LEShortField("ChannelFrequency", 0),
+            LEShortField("ChannelPlusFrequency", 0),
             lambda pkt: pkt.present and pkt.present.ChannelPlus),
         ConditionalField(
-            ByteField("ChannelNumber", 0),
+            ByteField("ChannelPlusNumber", 0),
             lambda pkt: pkt.present and pkt.present.ChannelPlus),
         # MCS
         ConditionalField(
diff --git a/scapy/main.py b/scapy/main.py
index 433a465b435..371f73b8389 100644
--- a/scapy/main.py
+++ b/scapy/main.py
@@ -17,10 +17,11 @@
 import gzip
 import glob
 import importlib
+import io
 import logging
-from random import choice
 import types
-import io
+import warnings
+from random import choice
 
 # Never add any global import, in main.py, that would trigger a warning message  # noqa: E501
 # before the console handlers gets added in interact()
@@ -83,7 +84,11 @@ def _read_config_file(cf, _globals=globals(), _locals=locals(), interactive=True
     """
     log_loading.debug("Loading config file [%s]", cf)
     try:
-        exec(compile(open(cf).read(), cf, 'exec'), _globals, _locals)
+        with open(cf) as cfgf:
+            exec(
+                compile(cfgf.read(), cf, 'exec'),
+                _globals, _locals
+            )
     except IOError as e:
         if interactive:
             raise
@@ -473,6 +478,9 @@ def interact(mydict=None, argv=None, mybanner=None, loglevel=logging.INFO):
         )
     log_scapy.addHandler(console_handler)
 
+    # We're in interactive mode, let's throw the DeprecationWarnings
+    warnings.simplefilter("always")
+
     from scapy.config import conf
     conf.interactive = True
     conf.color_theme = DefaultTheme()
diff --git a/scapy/packet.py b/scapy/packet.py
index c2596480cda..a4711d24282 100644
--- a/scapy/packet.py
+++ b/scapy/packet.py
@@ -14,6 +14,7 @@
 import itertools
 import copy
 import types
+import warnings
 
 from scapy.fields import StrField, ConditionalField, Emph, PacketListField, \
     BitField, MultiEnumField, EnumField, FlagsField, MultipleTypeField
@@ -51,8 +52,9 @@ def __repr__(self):
 class Packet(six.with_metaclass(Packet_metaclass, BasePacket,
                                 _CanvasDumpExtended)):
     __slots__ = [
-        "time", "sent_time", "name", "default_fields",
-        "overload_fields", "overloaded_fields", "fields", "fieldtype",
+        "time", "sent_time", "name",
+        "default_fields", "fields", "fieldtype",
+        "overload_fields", "overloaded_fields",
         "packetfields",
         "original", "explicit", "raw_packet_cache",
         "raw_packet_cache_fields", "_pkt", "post_transforms",
@@ -70,6 +72,7 @@ class Packet(six.with_metaclass(Packet_metaclass, BasePacket,
     ]
     name = None
     fields_desc = []
+    deprecated_fields = None
     overload_fields = {}
     payload_guess = []
     show_indent = 1
@@ -155,7 +158,13 @@ def __init__(self, _pkt=b"", post_transform=None, _internal=0, _underlayer=None,
                 continue
             self.fields[fname] = self.get_field(fname).any2i(self, value)
         # The remaining fields are unknown
-        for fname, _ in fields:
+        for fname in fields:
+            if self.deprecated_fields and fname in self.deprecated_fields:
+                # Resolve deprecated fields
+                value = fields[fname]
+                fname = self._resolve_alias(fname)
+                self.fields[fname] = self.get_field(fname).any2i(self, value)
+                continue
             raise AttributeError(fname)
         if isinstance(post_transform, list):
             self.post_transforms = post_transform
@@ -307,7 +316,18 @@ def copy(self):
         clone.time = self.time
         return clone
 
+    def _resolve_alias(self, attr):
+        new_attr, version = self.deprecated_fields[attr]
+        warnings.warn(
+            "%s has been deprecated in favor of %s since %s !" % (
+                attr, new_attr, version
+            ), DeprecationWarning
+        )
+        return new_attr
+
     def getfieldval(self, attr):
+        if self.deprecated_fields and attr in self.deprecated_fields:
+            attr = self._resolve_alias(attr)
         if attr in self.fields:
             return self.fields[attr]
         if attr in self.overloaded_fields:
@@ -317,6 +337,8 @@ def getfieldval(self, attr):
         return self.payload.getfieldval(attr)
 
     def getfield_and_val(self, attr):
+        if self.deprecated_fields and attr in self.deprecated_fields:
+            attr = self._resolve_alias(attr)
         if attr in self.fields:
             return self.get_field(attr), self.fields[attr]
         if attr in self.overloaded_fields:
@@ -334,6 +356,8 @@ def __getattr__(self, attr):
         return v
 
     def setfieldval(self, attr, val):
+        if self.deprecated_fields and attr in self.deprecated_fields:
+            attr = self._resolve_alias(attr)
         if attr in self.default_fields:
             fld = self.get_field(attr)
             if fld is None:
diff --git a/test/fields.uts b/test/fields.uts
index 0659b0e5073..87e0d630b3f 100644
--- a/test/fields.uts
+++ b/test/fields.uts
@@ -1809,6 +1809,37 @@ r = m.m2i(None, data_129["str_with_fx"])
 assert(r == data_129["extended"])
 
 
+############
+############
++ Deprecated fields in Packet
+~ deprecated
+
+= Field Deprecation test
+
+class TestPacket(Packet):
+    fields_desc = [
+        ByteField("a", 0),
+        LEShortField("b", 15),
+    ]
+    deprecated_fields = {
+        "dpr": ("a", "1.0"),
+        "B": ("b", "1.0"),
+    }
+
+pkt = TestPacket(a=2, B=3)
+assert pkt.B == 3
+assert pkt.b == 3
+assert pkt.a == 2
+
+import warnings
+
+with warnings.catch_warnings(record=True) as w:
+    warnings.simplefilter("always")
+    assert pkt.dpr == 2
+    assert len(w) == 1
+    assert issubclass(w[-1].category, DeprecationWarning)
+
+
 ############
 ############
 + FCSField
diff --git a/test/regression.uts b/test/regression.uts
index 07c08b5efc4..bfcb5f141cd 100644
--- a/test/regression.uts
+++ b/test/regression.uts
@@ -1076,6 +1076,7 @@ r = RadioTap(data)
 r = RadioTap(raw(r))
 assert r.dBm_AntSignal == -59
 assert r.ChannelFrequency == 5785
+assert r.ChannelPlusFrequency == 5785
 assert r.present == 3410027
 assert r.A_MPDU_ref == 2821
 assert r.KnownVHT == 511


Code review comments:
diff_hunk: @@ -70,6 +72,7 @@ class Packet(six.with_metaclass(Packet_metaclass, BasePacket,
     ]
     name = None
     fields_desc = []
+    deprecated_fields = None
path: scapy/packet.py
text: @user1:
```suggestion
    deprecated_fields = {}
```

diff_hunk: @@ -155,7 +158,13 @@ def __init__(self, _pkt=b"", post_transform=None, _internal=0, _underlayer=None,
                 continue
             self.fields[fname] = self.get_field(fname).any2i(self, value)
         # The remaining fields are unknown
-        for fname, _ in fields:
+        for fname in fields:
+            if self.deprecated_fields and fname in self.deprecated_fields:
path: scapy/packet.py
text: @user1:
```suggestion
            if fname in self.deprecated_fields:
```